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

Avoid contention on Instant::now() calls #82

Merged
merged 6 commits into from
Mar 22, 2023
Merged

Avoid contention on Instant::now() calls #82

merged 6 commits into from
Mar 22, 2023

Conversation

loyd
Copy link
Contributor

@loyd loyd commented Mar 21, 2023

  • Instant::duration_since now saturates to zero.
  • Add the contention benchmark and store results in benches/README.md.
  • Instant::elapsed is added to make Instant drop-in replacement of std::time::Instant.
  • Add generation of links to diffs in CHANGELOG.md (according to keepachangelog spec).

Results (performed on an AMD Ryzen 7 4800HS CPU):

group            main                                    no-contention
-----            ----                                    -------------
quanta/now/1     1.33     18.8±9.87ns        ? ?/sec     1.00     14.2±7.40ns        ? ?/sec
quanta/now/10    13.38  345.7±54.28ns        ? ?/sec     1.00    25.8±10.19ns        ? ?/sec
quanta/now/11    15.72  394.6±37.46ns        ? ?/sec     1.00     25.1±8.89ns        ? ?/sec
quanta/now/12    17.74  457.2±59.94ns        ? ?/sec     1.00     25.8±9.02ns        ? ?/sec
quanta/now/2     3.23    53.6±12.42ns        ? ?/sec     1.00     16.6±5.66ns        ? ?/sec
quanta/now/3     6.11   100.7±35.62ns        ? ?/sec     1.00     16.5±5.76ns        ? ?/sec
quanta/now/4     9.31   160.2±43.22ns        ? ?/sec     1.00     17.2±5.69ns        ? ?/sec
quanta/now/5     10.32  182.0±38.35ns        ? ?/sec     1.00     17.6±5.71ns        ? ?/sec
quanta/now/6     11.92  205.4±61.60ns        ? ?/sec     1.00     17.2±5.86ns        ? ?/sec
quanta/now/7     13.92  244.1±65.22ns        ? ?/sec     1.00     17.5±6.55ns        ? ?/sec
quanta/now/8     15.56  269.7±60.34ns        ? ?/sec     1.00     17.3±6.17ns        ? ?/sec
quanta/now/9     12.79  309.2±87.00ns        ? ?/sec     1.00     24.2±9.47ns        ? ?/sec
stdlib/now/1     1.03  1165.6±142.16ns        ? ?/sec    1.00  1126.9±71.75ns        ? ?/sec
stdlib/now/10    1.00  1368.3±30.03ns        ? ?/sec     1.01  1386.1±75.49ns        ? ?/sec
stdlib/now/11    1.00  1401.8±70.33ns        ? ?/sec     1.00  1401.1±68.81ns        ? ?/sec
stdlib/now/12    1.00  1389.6±48.94ns        ? ?/sec     1.00  1387.9±38.73ns        ? ?/sec
stdlib/now/2     1.00  1358.9±105.26ns        ? ?/sec    1.05  1431.1±147.48ns        ? ?/sec
stdlib/now/3     1.00  1380.8±55.35ns        ? ?/sec     1.00  1382.1±73.67ns        ? ?/sec
stdlib/now/4     1.05  1428.2±133.49ns        ? ?/sec    1.00  1363.3±65.90ns        ? ?/sec
stdlib/now/5     1.01  1391.0±117.71ns        ? ?/sec    1.00  1374.8±60.66ns        ? ?/sec
stdlib/now/6     1.02  1365.4±63.80ns        ? ?/sec     1.00  1344.3±46.68ns        ? ?/sec
stdlib/now/7     1.02  1387.7±76.32ns        ? ?/sec     1.00  1363.6±53.60ns        ? ?/sec
stdlib/now/8     1.01  1411.2±115.90ns        ? ?/sec    1.00  1398.2±92.98ns        ? ?/sec
stdlib/now/9     1.03  1416.0±93.12ns        ? ?/sec     1.00  1374.0±56.60ns        ? ?/sec

Closes #81

loyd added 3 commits March 21, 2023 12:04
Saturate in `duration_since` and `sub` instead of using global atomic variable to guarantee monotonicity of clocks.

See metrics-rs#81 for more details.

Closes metrics-rs#81
@loyd
Copy link
Contributor Author

loyd commented Mar 21, 2023

During development:

  • The "test_reference_source_calibration" test fails (even on the main branch) with mean=1124.1603625000016
  • The "test_recent" test fails sometimes (even on the main branch). Now, saturation hides the problem and the test passes always. Need to investigate, actually.
  • #![deny(clippy::all)] is inconvenient during development, I suggest to add -D warnings in CI instead.

@loyd loyd mentioned this pull request Mar 21, 2023
@tobz
Copy link
Member

tobz commented Mar 22, 2023

  • The "test_reference_source_calibration" test fails (even on the main branch) with mean=1124.1603625000016

This test is sort of inherently flaky, so I wouldn't worry about it. I'll likely end up marking it as #[ignore] before cutting the next release.

  • The "test_recent" test fails sometimes (even on the main branch). Now, saturation hides the problem and the test passes always. Need to investigate, actually.

Hmm, yeah, interesting. 🤔

@tobz tobz merged commit 2eb5cd2 into metrics-rs:main Mar 22, 2023
@loyd
Copy link
Contributor Author

loyd commented Mar 23, 2023

Thanks! Waiting for the new release to move back to upstream instead of a fork.

@tobz
Copy link
Member

tobz commented Mar 24, 2023

FWIW, I tracked down and fixed the issue with the test_recent test, which was related to the multiple tests, test_recent included, interacting with the global recent time in a way that with the right ordering, it would change the value that was being asserted against.

@tobz
Copy link
Member

tobz commented Mar 24, 2023

@loyd This PR is now released as of quanta@v0.11.0.

Thanks again for your contribution. 👍🏻

tabVersion pushed a commit to tabVersion/quanta that referenced this pull request Oct 29, 2024
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.

Much slower than std version in case of high contention
2 participants