Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CopyableThreadContextElement implementation #3227

Merged
merged 26 commits into from
Apr 4, 2022
Merged

Conversation

qwwdfsad
Copy link
Contributor

No description provided.

@qwwdfsad qwwdfsad marked this pull request as ready for review March 30, 2022 15:59
@qwwdfsad qwwdfsad requested a review from dkhalanskyjb March 30, 2022 15:59
@qwwdfsad
Copy link
Contributor Author

PTAL, especially at how clear the documentation (both public and internal) is.

I've measured the performance impact and for a classic ping-pong application an additional context scan is negligible and practically undetectable.

It neither means that it's zero nor that we won't encounter any issues with it in the future. The biggest potential impact is our future selves -- I can easily imagine 30-50+ elements context in enterprise-grade applications, but it requires a completely different solution, namely rework of the context elements themselves (now being a straightforward immutable map is not really the fastest solution, especially with weird plus and minusKey contracts)

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't look at the tests yet, but I did try breaking the code in a few non-obvious places to ensure that these are tested.

kotlinx-coroutines-core/jvm/src/CoroutineContext.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/jvm/src/CoroutineContext.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/jvm/src/CoroutineContext.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/jvm/src/ThreadContextElement.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/jvm/src/ThreadContextElement.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/jvm/src/ThreadContextElement.kt Outdated Show resolved Hide resolved
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked through the tests as well.

qwwdfsad and others added 15 commits April 1, 2022 14:21
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
…t.kt

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
@qwwdfsad qwwdfsad requested a review from dkhalanskyjb April 1, 2022 16:45
@qwwdfsad qwwdfsad mentioned this pull request Apr 1, 2022
kotlinx-coroutines-core/jvm/src/ThreadContextElement.kt Outdated Show resolved Hide resolved
Comment on lines 134 to 135
* A coroutine using this mechanism can safely call Java code that assumes it's called using a
* `Thread`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the meaning here. Is it possible to call any Java code without using a Thread?

kotlinx-coroutines-core/jvm/src/ThreadContextElement.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/jvm/src/ThreadContextElement.kt Outdated Show resolved Hide resolved
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
qwwdfsad and others added 3 commits April 2, 2022 16:18
@qwwdfsad qwwdfsad requested a review from dkhalanskyjb April 4, 2022 08:41
@qwwdfsad qwwdfsad requested a review from dkhalanskyjb April 4, 2022 09:40
@qwwdfsad qwwdfsad merged commit a5dd74b into develop Apr 4, 2022
@qwwdfsad qwwdfsad deleted the coroutines-tracing branch April 4, 2022 10:57
pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
New approach eagerly copies corresponding elements to avoid accidental top-level reuse and also provides merge capability in case when an element is being overwritten. Merge capability is crucial in tracing scenarios to properly preserve the state of linked thread locals 


Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants