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

CopyableThreadContextElement implementation #3227

merged 26 commits into from Apr 4, 2022

Conversation

qwwdfsad
Copy link
Member

@qwwdfsad qwwdfsad commented Mar 30, 2022

No description provided.

@qwwdfsad qwwdfsad marked this pull request as ready for review Mar 30, 2022
@qwwdfsad qwwdfsad requested a review from dkhalanskyjb Mar 30, 2022
@qwwdfsad
Copy link
Member Author

@qwwdfsad qwwdfsad commented Mar 30, 2022

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
Member

@dkhalanskyjb dkhalanskyjb left a comment

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
Member

@dkhalanskyjb dkhalanskyjb left a comment

Looked through the tests as well.

qwwdfsad and others added 15 commits Apr 1, 2022
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 Apr 1, 2022
@qwwdfsad qwwdfsad mentioned this pull request Apr 1, 2022
kotlinx-coroutines-core/jvm/src/ThreadContextElement.kt Outdated Show resolved Hide resolved
* A coroutine using this mechanism can safely call Java code that assumes it's called using a
* `Thread`.
Copy link
Member

@dkhalanskyjb dkhalanskyjb Apr 1, 2022

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 Apr 2, 2022
@qwwdfsad qwwdfsad requested a review from dkhalanskyjb Apr 4, 2022
@qwwdfsad qwwdfsad requested a review from dkhalanskyjb Apr 4, 2022
@qwwdfsad qwwdfsad merged commit a5dd74b into develop Apr 4, 2022
1 check passed
@qwwdfsad qwwdfsad deleted the coroutines-tracing branch Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants