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

Properly handle the race condition #164

Merged
merged 2 commits into from
Nov 25, 2022
Merged

Properly handle the race condition #164

merged 2 commits into from
Nov 25, 2022

Conversation

mpenkov
Copy link
Collaborator

@mpenkov mpenkov commented Nov 20, 2022

Use synchronization primitives instead of sleeping. This eliminates the need for the timeout parameter. The affected class is effectively internal, so this "should" not affect regular users.

Also fixed the tests: they didn't clean up after themselves.

Added benchmarks.

Fixes #152

Use synchronization primitives instead of sleeping.
This eliminates the need for the timeout parameter.
The affected class is effectively internal, so this "should" not affect
regular users.

Also fixed the tests: they didn't clean up after themselves.

Added benchmarks.

Fixes #152
Copy link
Owner

@piskvorky piskvorky left a comment

Choose a reason for hiding this comment

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

What does the new benchmark say? Did this PR affect performance much?

@mpenkov
Copy link
Collaborator Author

mpenkov commented Nov 21, 2022

On MacOS, there is a significant difference.

Before (note time is in seconds):

-------------------------------------------- benchmark: 1 tests --------------------------------------------
Name (time in s)        Min      Max     Mean  StdDev   Median     IQR  Outliers     OPS  Rounds  Iterations
------------------------------------------------------------------------------------------------------------
test                 9.8556  11.3276  10.8173  0.5690  10.9809  0.5847       1;0  0.0924       5           1
------------------------------------------------------------------------------------------------------------

After (note time is in ms):

------------------------------------------------ benchmark: 1 tests ------------------------------------------------
Name (time in ms)          Min       Max      Mean   StdDev    Median      IQR  Outliers     OPS  Rounds  Iterations
--------------------------------------------------------------------------------------------------------------------
test                  338.2111  401.0051  355.2427  26.0633  344.9050  23.6982       1;1  2.8150       5           1
--------------------------------------------------------------------------------------------------------------------

On Linux, there is no difference in performance, but I could not reproduce the problem, the benchmark times are about the same (~300ms).

@piskvorky
Copy link
Owner

piskvorky commented Nov 21, 2022

Oh wow, 30x slower now? We'll have to look into that, that's too much. I'll try to check too (I have a macbook).

@mpenkov
Copy link
Collaborator Author

mpenkov commented Nov 22, 2022

No, it's the other way around. Before it was 10s, now it's 338ms (0.338s).

@piskvorky
Copy link
Owner

Oh yes, I missed the bracket with "note" of course :) Sorry.

Great job!

@mpenkov mpenkov merged commit 8606ca4 into master Nov 25, 2022
@piskvorky piskvorky deleted the synchro branch November 25, 2022 13:45
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.

Version 2.0.0 significantly slower
2 participants