chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.78k stars 419 forks source link

How should we control parallel-safety for `list`? #18097

Open e-kayrakli opened 3 years ago

e-kayrakli commented 3 years ago

list is not parallel-safe by default. However, you can opt-in for parallel-safety by new list(int, parSafe=true). Currently, enabling parallel safety makes some of the list methods give compiler errors, if they are used. These methods are; this, first and last. Where the rationale is that the reference they return could be invalidated by a parallel task if it happens to remove something from the list.

There are some questions about how parallel-safety should be handled by list. Some options are:

  1. Make list a non-parallel safe structure, add a new data structure that's parallel-safe
  2. Make this, first and last compile, but add warnings in the documentation that they should be used with care in parallel contexts.
  3. Make parallel-safety less prominent: a. Control parallel-safety in method granularity instead of type granularity b. Instead of parSafe, call it locking or something less assuring
  4. ...

Personally, I find creating a separate data structure only for parallel-safety a bit of an overkill. And I am not quite happy with the current state of methods giving compiler errors with parallel-safety on. I would like to consider 2 and maybe some of 3.

e-kayrakli commented 3 years ago

A related list issue that can be moot if we were to choose 1: https://github.com/chapel-lang/chapel/issues/16707

mppf commented 3 years ago

As I commented on #18095, I'm for creating a separate data structure for the parallel-safe one. Even if the data structure is locking, that doesn't mean that it does enough to ensure parallel safety in an application. (In particular updating the element might need to be done under a lock). So I think I'd rather see an approach (like @dlongnecke-cray was describing) using context managers to allow users to easily write code using locks.

lydia-duncan commented 3 years ago

I'd be worried that controlling parallel safety on a method granularity would not be sufficient for safety without underlying field support and at that point it seems pointless to not make the parallel safety part of the user interface for the type as a whole

dlongnecke-cray commented 3 years ago

It seems like we're trying to decide what parSafe means here. If we're just trying to describe if locking is used, I'd agree that we should just rename parSafe -> locking and call it a day.

However, with the advent of lock guards that can be used within a context manager, I think something like locking is too inflexible to be useful.

A user might like to select the kind of lock that is used depending on the workload. Since the list controls the lock, this is either not possible, or would require us to extend locking into an enum that can probably only contain standard module locks. It's not a very flexible approach either way.

And while it is true that having the list control the lock can lead to a more performant/integrated/fine-grained locking scheme, I would argue that:

TLDR: In the longterm, I think it best if we plan to:

This will allow us to deprecate several methods on list and dramatically simplify the implementation of the type.

In the short term, a transitional option is to rename parSafe to locking, as well as to revert compiler errors on several methods that were introduced only to make list parallel-safe. Then, after we're confident in the lock guard feature set, we can deprecate the locking flag entirely.

lydia-duncan commented 1 year ago

We decided to replace the parSafe field with a separate, parallel implementation of list in the future. Removing the 2.0 label as a result, but leaving this open until we implement that decision