Skip to content

Properly handle the race condition - #164

Merged
mpenkov merged 2 commits into
masterfrom
synchro
Nov 25, 2022
Merged

Properly handle the race condition#164
mpenkov merged 2 commits into
masterfrom
synchro

Conversation

@mpenkov

@mpenkov mpenkov commented Nov 20, 2022

Copy link
Copy Markdown
Collaborator

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
@mpenkov
mpenkov requested a review from piskvorky November 20, 2022 14:56

@piskvorky piskvorky left a comment

Copy link
Copy Markdown
Owner

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

mpenkov commented Nov 21, 2022

Copy link
Copy Markdown
Collaborator Author

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

piskvorky commented Nov 21, 2022

Copy link
Copy Markdown
Owner

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

mpenkov commented Nov 22, 2022

Copy link
Copy Markdown
Collaborator Author

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

@piskvorky

Copy link
Copy Markdown
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

2 participants