Skip to content

Restore thread context elements when directly resuming to parent #1577

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

Merged
merged 2 commits into from
Feb 1, 2021

Conversation

elizarov
Copy link
Contributor

@elizarov elizarov commented Sep 27, 2019

This fix solves the problem of restoring thread-context when returning to another context in undispatched way. It impacts suspend/resume performance of coroutines that use ThreadContextElement since we have to walk up the coroutine completion stack in search for
parent UndispatchedCoroutine. However, there is a fast-path to ensure that there is no performance impact in cases when ThreadContextElement is not used by a coroutine.

Fixes #985

@elizarov elizarov force-pushed the thread-context-restore branch from 9d9dbff to ab04731 Compare September 30, 2019 15:23
@elizarov elizarov changed the title Restore thread context elements when directly resuming to parent [DRAFT] Restore thread context elements when directly resuming to parent Nov 28, 2019
@elizarov elizarov force-pushed the thread-context-restore branch 3 times, most recently from 1cebb93 to fb72911 Compare December 19, 2019 11:48
@elizarov elizarov changed the title [DRAFT] Restore thread context elements when directly resuming to parent Restore thread context elements when directly resuming to parent Dec 19, 2019
@elizarov elizarov marked this pull request as ready for review December 19, 2019 11:49
@elizarov elizarov requested a review from qwwdfsad December 19, 2019 11:49
@qwwdfsad qwwdfsad force-pushed the develop branch 3 times, most recently from 4a49830 to aff8202 Compare March 10, 2020 17:27
@elizarov elizarov marked this pull request as draft May 27, 2020 07:14
@elizarov elizarov changed the title Restore thread context elements when directly resuming to parent [DRAFT] Restore thread context elements when directly resuming to parent May 27, 2020
@elizarov elizarov force-pushed the thread-context-restore branch from fb72911 to c02fe59 Compare May 27, 2020 07:19
@elizarov
Copy link
Contributor Author

Marking it as a DRAFT. It works, but we need to evaluate its performance impact.

@qwwdfsad qwwdfsad force-pushed the thread-context-restore branch from 1d02d27 to 35c80c1 Compare December 1, 2020 16:07
@qwwdfsad qwwdfsad marked this pull request as ready for review December 1, 2020 17:22
@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Dec 1, 2020

Performance impact on our stress for context and channels

Benchmark                                     Score change                                      
ChannelSinkBenchmark.channelPipeline                  -1%
ChannelSinkBenchmark.channelPipelineOneThreadLocal   -11%
ChannelSinkBenchmark.channelPipelineTwoThreadLocals   -7%

@qwwdfsad qwwdfsad removed the For 1.4 label Dec 2, 2020
@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Dec 2, 2020

More benchmarks

Baseline:

Benchmark                            Mode  Cnt  Score   Error  Units
ChannelSinkDepthBenchmark.depth1     avgt   10  3.025 ± 0.085  ms/op
ChannelSinkDepthBenchmark.depth10    avgt   10  2.987 ± 0.082  ms/op
ChannelSinkDepthBenchmark.depth100   avgt   10  2.964 ± 0.041  ms/op
ChannelSinkDepthBenchmark.depth1000  avgt   10  3.012 ± 0.067  ms/op

Original change:

Benchmark                            Mode  Cnt  Score   Error  Units
ChannelSinkDepthBenchmark.depth1     avgt   10   3.400 ± 0.069  ms/op
ChannelSinkDepthBenchmark.depth10    avgt   10   3.793 ± 0.074  ms/op
ChannelSinkDepthBenchmark.depth100   avgt   10   5.147 ± 0.462  ms/op
ChannelSinkDepthBenchmark.depth1000  avgt   10  25.376 ± 0.851  ms/op

Recent change:

Benchmark                            Mode  Cnt  Score   Error  Units
ChannelSinkDepthBenchmark.depth1     avgt   10  3.165 ± 0.124  ms/op
ChannelSinkDepthBenchmark.depth10    avgt   10  3.141 ± 0.060  ms/op
ChannelSinkDepthBenchmark.depth100   avgt   10  3.178 ± 0.071  ms/op
ChannelSinkDepthBenchmark.depth1000  avgt   10  3.214 ± 0.105  ms/op

@qwwdfsad qwwdfsad changed the title [DRAFT] Restore thread context elements when directly resuming to parent Restore thread context elements when directly resuming to parent Dec 3, 2020
@qwwdfsad qwwdfsad force-pushed the thread-context-restore branch from 241aa0e to df05db6 Compare December 3, 2020 13:27
@qwwdfsad qwwdfsad force-pushed the thread-context-restore branch from df05db6 to ad07b1b Compare February 1, 2021 14:03
This fix solves the problem of restoring thread-context when
returning to another context in undispatched way.

It impacts suspend/resume performance of coroutines that use ThreadContextElement and undispatched coroutines.

The kotlinx.coroutines code poisons the context with special 'UndispatchedMarker' element and linear lookup is performed only when the marker is present. The code also contains description of an alternative approach in order to save a linear lookup in complex coroutines hierarchies.

Fast-path of coroutine resumption is slowed down by a single context lookup.

Fixes #985

Co-authored-by: Roman Elizarov <[email protected]>
@qwwdfsad qwwdfsad force-pushed the thread-context-restore branch from ad07b1b to 5dc55a6 Compare February 1, 2021 14:06
@qwwdfsad qwwdfsad merged commit 727c38f into develop Feb 1, 2021
@qwwdfsad qwwdfsad deleted the thread-context-restore branch February 1, 2021 15:19
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