search

Kotlin / kotlinx.coroutines

Library support for Kotlin coroutines
Apache License 2.0
12.95k stars 1.84k forks source link

@FlowPreview on `DEFAULT_CONCURRENCY` #3938

Open hoc081098 opened 10 months ago

hoc081098 commented 10 months ago

Hello maintainers,

I see DEFAULT_CONCURRENCY marked with DEFAULT_CONCURRENCY, but Flow<Flow<T>>.flattenMerge(concurrency: Int = DEFAULT_CONCURRENCY) is not marked with FlowPreview.

Is there any reason to keep @FlowPreview on DEFAULT_CONCURRENCY? Can we remove @FlowPreview on DEFAULT_CONCURRENCY?

Thanks🙏

dkhalanskyjb commented 7 months ago

Hi!

Is there any reason to keep @FlowPreview on DEFAULT_CONCURRENCY?

Like with all other FlowPreview entities, this gives us the freedom to rename, move, and remove this entity (unless this affects flattenMerge). If you're using DEFAULT_CONCURRENCY, the forward compatibility of your code is not guaranteed.

Can we remove @FlowPreview on DEFAULT_CONCURRENCY?

If we decide that, yeah, it's a good API entry, and we want to keep it, then yes.

Here are some thoughts.

https://grep.app/search?q=DEFAULT_CONCURRENCY&case=true&words=true&filter[lang][0]=Kotlin clearly shows the typical use case for this variable: defining one's own flattenMerge variant or wrapper. This could be useful: this way, both standalone flattenMerge and the ones wrapped in some other function behave the same way when kotlinx.coroutines.flow.defaultConcurrency is changed. So, if the system property is useful, then DEFAULT_CONCURRENCY is also useful.

The big question is, does kotlinx.coroutines.flow.defaultConcurrency ever get changed?

This property was introduced in https://github.com/Kotlin/kotlinx.coroutines/pull/1234 alongside fixes to https://github.com/Kotlin/kotlinx.coroutines/issues/1233. I don't see any user demand for kotlinx.coroutines.flow.defaultConcurrency (see https://grep.app/search?q=DEFAULT_CONCURRENCY_PROPERTY_NAME, https://grep.app/search?q=kotlinx.coroutines.flow.defaultConcurrency, and the same queries in Google); frankly, I can't even think of any use cases for it. Let's say you have a large codebase that uses flattenMerge throughout. In one place, you process flows of millions of elements, while in another, there are only 10-20 elements in each. If you change the flattenMerge concurrency throughout your system via the system property, you're making it more suitable for one part of the code at the expense of another. If you're very serious about tuning the performance of your code, you'll probably define separate variables for these use cases yourself and tweak them separately.

Unless I'm missing something, I think it's unfortunate that we introduced this variable: if we hardcoded 16 to flattenMerge, we could later extract it to a separate variable if we saw any demand for this, but now, we'll have to think of the best-effort migration path if we decide to remove it, as per FlowPreview guarantees.

@qwwdfsad, I didn't find mentions of this system property in our design documents. Do you happen to remember why it was introduced?