Add optional name parameter to .limitedParallelism

[qwwdfsad]

Fixes #4023

[qwwdfsad]

Conceptually, the PR is ready.

Practically, I would like to polish and figure some binary compatibility details before merging it and ensure it is a reasonable thing to do in a non-major release

[dkhalanskyjb]

What are the reasons to take the non-orthogonal path here? I think it's much more intuitive to have names that are unconditionally applied.

[qwwdfsad]

My line of thinking is the following:

• The implementation allows returning this which is a nice property to have -- you don't have to care about the underlying/given pool capacity if not necessary
  • Also, mistakes are much more forgiving. E.g. now for cases like that nothing really happens, with the name forcing to return something else it's suddenly more subtle
• I don't really like the idea of being able to give dispatchers names when no new dispatcher is effectively created -- the probability of accidental aliasing just increases with not that many benefits
• Returning LimitedDispatcher for the sole purpose of naming is a suboptimal idea -- it slightly changes the observable behaviour and introduces some overhead. It kind of forces us to introduce one more implementation (NamedDispatcher?) and maintain it

[dkhalanskyjb]

The implementation allows returning this which is a nice property to have

This can be preserved in the case of parallelism >= dispatcher.parallelism && (newName == null || newName == dispatcher.name).

I don't really like the idea of being able to give dispatchers names when no new dispatcher is effectively created -- the probability of accidental aliasing just increases with not that many benefits

Two bureaucratic benefits come to mind immediately:

• Principle of least surprise: specifically, if the user did provide a name for some reason, it's surprising to see it ignored.
• Principle of orthogonality: conceptually, now, the two arguments to limitedParallelism are linked together, while they could not be that.

The second concern is practical, I think. Let's look at the issue statement:

We have specific dispatchers, and in coroutine dumps they all look like LimitedDispatcher@xxxxxxxx, and it's not immediately clear where the dispatcher comes from.

I see two possible cases:

1. "Dispatchers.Default.limitedParallelism(4)" is already good enough. Then why would we add a new name parameter at all?
2. "Dispatchers.Default.limitedParallelism(4)" is not good enough, and something like IndexBuilding is needed instead. In this case, why does the name revert to the not-good-enough "Dispatchers.Default.limitedParallelism(4)" option when there are few CPU cores?

As to the accidental aliasing issue: what dangers do you see in it?

Returning LimitedDispatcher for the sole purpose of naming is a suboptimal idea -- it slightly changes the observable behaviour and introduces some overhead. It kind of forces us to introduce one more implementation (NamedDispatcher?) and maintain it

That's true. Why is this an issue, though? Is an extra call stack frame per dispatch a significant problem?

[qwwdfsad]

Note: discussed it internally and the rationale will be reflected in the public design doc

[dkhalanskyjb]

LimitedDispatcher having its own name instead of always delegating this behavior to a NamedDispatcher is a nice idea.

There's still potential for tiny optimizations: for example, we could also override limitedParallelism on NamedDispatcher to allow in-depth inspection of dispatcher (throwing away the NamedDispatcher wrapper if it's no longer needed), or we could join nested NamedDispatchers into one, etc.

I'm not sure, though, that this would be useful to anyone, and certainly not to a point where this would block adding this useful change.

[dkhalanskyjb]

(Minor) Since checkParallelism was already called, we could avoid calling super here, instead directly constructing a LimitedDispatcher.

[qwwdfsad]

In things like that, I prefer leaving the double-check, but ensuring that the code is reused and has less entry point