Function containing atomic operations should be private

[mvicsokolova]

Calling a function containing atomic operations from a different file results in the failure of Native incremental compilation.

Made BufferedChannel#sendImpl function private.

This is a WA for KT-65554

Maybe we could refactor this in a better way though.

[homuroll]

Just to clarify: this fixes problem during K/N incremental compilation where compilation is done per-file and it is unsupported to have cross-file field accesses, which are generated by atomicfu plugin/volatile intrinsics lowering.

[dkhalanskyjb]

What did this poor thing do to deserve being commented out?

[mvicsokolova]

Fixed, I did not mean it)

[dkhalanskyjb]

What are we trying to achieve with this PR? The problem only arises with -Pkotlin.incremental.native=true, right? I would understand it better if this PR also enabled kotlin.incremental.native for the project so we could in theory benefit from incremental compilation (so the goal of the PR would be to improve the performance of incremental native compilation, and working around the bug would just be a necessary step for that), but as is… as a doctor would say, if this flag brings you pain, just don't enable this flag.

[mvicsokolova]

@homuroll Am I right that if incremental compilation is not enabled for kotlinx.coroutines, then this issue will not cause any compilation problems for users that depend on coroutines and use K/N incremental compilation?

[mvicsokolova]

And is there a reason, why we can not enable K/N incremental compilation now?

[SvyatoslavScherbina]

What are we trying to achieve with this PR?

Generally, we are trying to avoid having incorrect IR in kotlinx.coroutines klibs. See KT-65554 for more details.
Problems with Native incremental compilation are just a symptom. Basically, the compiler can fail in different ways because of the incorrect IR when compiling any binary involving kotlinx.coroutines. In particular, in projects depending on the library.

The problem only arises with -Pkotlin.incremental.native=true, right?

Yes, but an important thing to note is that the problem might arise in any project (with the flag enabled) depending on kotlinx.coroutines. In other words, this PR might improve Native incremental compilation stability for many users.

I would understand it better if this PR also enabled kotlin.incremental.native for the project so we could in theory benefit from incremental compilation (so the goal of the PR would be to improve the performance of incremental native compilation, and working around the bug would just be a necessary step for that)

Well, this PR is simply a prerequisite for enabling Native incremental compilation in kotlinx.coroutines. After merging it, you can try to turn the flag on locally and see if it works for you. -Pkotlin.incremental.native=true is still experimental, there might be bugs, sometimes it doesn't provide a significant compilation time improvement. We had no chance to evaluate its effect on kotlinx.coroutines because of the issue this PR addresses.

Am I right that if incremental compilation is not enabled for kotlinx.coroutines, then this issue will not cause any compilation problems for users that depend on coroutines and use K/N incremental compilation?

Not quite, please see above.

And is there a reason, why we can not enable K/N incremental compilation now?

You can try, but please note its status. That said, I think that trying it out in kotlinx.coroutines is a good idea.

[dkhalanskyjb]

@SvyatoslavScherbina to sum it up: there is a bug somewhere between Kotlin/Native and atomicfu, but because fixing that bug could take a long time, as a quick workaround, we should merge this pull request so that the end users are less likely to be affected by it. Did I get this right?

If so, just how critical is this bug? We'd like to publish a release today. Should we try to make it a last-minute addition?

See KT-65554 for more details.

Is this link correct? I only see information about incremental compilation failing and a description of how this PR would fix the failure.

[SvyatoslavScherbina]

to sum it up: there is a bug somewhere between Kotlin/Native and atomicfu, but because fixing that bug could take a long time, as a quick workaround, we should merge this pull request so that the end users are less likely to be affected by it. Did I get this right?

I would say that the bug is in atomicfu compiler plugin, since it generates incorrect IR. Everything else is true.

If so, just how critical is this bug?

Not critical, but it hinders Kotlin/Native incremental compilation adoption.

We'd like to publish a release today. Should we try to make it a last-minute addition?

It would be nice, but not absolutely necessary.

Is this link correct? I only see information about incremental compilation failing and a description of how this PR would fix the failure.

Yes, the link is correct. As I said, it provides more details about how this IR is incorrect (field access from another file), what causes it (atomicfu compiler plugin), how to reproduce this with a user-visible effect and how it affects K/N in practice.

[dkhalanskyjb]

Got it, thanks. We already published a new release and didn't plan another one soon, so we can probably wait for this to be fixed on the side of atomicfu, or revisit this before the next release if the fix is not available by then. @mvicsokolova, WDYT?

[mvicsokolova]

I think, we should merge this commit now, as this issue may cause problems while compiling a project that depends on kotlinx.coroutines and uses incremental compilation.

I'll fix this cross-file access to the field in the atomicfu compiler plugin. And as soon, as we update to the corresponding Kotlin version, we'll be able to revert this WA.

[dkhalanskyjb]

Could you then please drop a comment above trySendDropOldest to explain why the function's in the wrong place? Also, do we know for sure that there are no other places in the codebase with the same problem that just didn't affect the incremental compilation for some reason?

[mvicsokolova]

The issue was caused by the call of an inline function, which invokes atomic operations, outside of the parent class. Seems like there no more cases like that left.

[dkhalanskyjb]

The change seems safe: all the symbols are resolved to the same entities.