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

chore: prefer atomic.Pointer to atomic.Value #338

Merged
merged 2 commits into from
Jul 21, 2024

Conversation

arnehormann
Copy link
Collaborator

@arnehormann arnehormann commented Jul 20, 2024

This improves the situation by closing down the api and enforcing the required type.
It also slightly reduces allocations and memory use and - at least on my system - improves benchmarking time for the whole system from 35.788s to 32.588s (but only one run each and 2 days apart...).

@arnehormann arnehormann changed the title prefer atomic.Pointer over atomic.Value prefer atomic.Pointer to atomic.Value Jul 20, 2024
@arnehormann arnehormann changed the title prefer atomic.Pointer to atomic.Value chore: prefer atomic.Pointer to atomic.Value Jul 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.56%. Comparing base (b0b50d8) to head (c637f69).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #338   +/-   ##
=======================================
  Coverage   96.56%   96.56%           
=======================================
  Files          18       18           
  Lines        1863     1863           
=======================================
  Hits         1799     1799           
  Misses         36       36           
  Partials       28       28           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@timbray timbray left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I wonder how I managed to not notice atomic.Pointer. Will leave a little while for pushing in case one of our other Go experts has an opinion.

@timbray
Copy link
Owner

timbray commented Jul 20, 2024

BTW @arnehormann unless you object I will add you as contributor so you can run CI etc.

@arnehormann
Copy link
Collaborator Author

arnehormann commented Jul 20, 2024

I don't object, but I don't know how often or how much I'll contribute after these PRs. Not that I won't, just that I don't know yet.
Thanks for your trust!

@timbray
Copy link
Owner

timbray commented Jul 20, 2024

I don't object, but I don't know how often or how much I'll contribute after these PRs. Not that I won't, just that I don't know yet. Thanks for your trust!

Well, as long as you have a couple of PRs in flight, you should be able to trigger the CI/CD without waiting for me to wake up in West Coast time and press the "Run CI" button.

@arnehormann
Copy link
Collaborator Author

I don't object, but I don't know how often or how much I'll contribute after these PRs. Not that I won't, just that I don't know yet. Thanks for your trust!

Well, as long as you have a couple of PRs in flight, you should be able to trigger the CI/CD without waiting for me to wake up in West Coast time and press the "Run CI" button.

That... is indeed quite nice. I needed a lot of roundtrips until I broke no rules and a rather stupid blindness bug was squashed in numbits...

@timbray
Copy link
Owner

timbray commented Jul 20, 2024

That... is indeed quite nice. I needed a lot of roundtrips until I broke no rules and a rather stupid blindness bug was squashed in numbits...

Our CI/CD setup is due to @embano1 and is quite effective, although as you'll see if you watch the ignore-case PR, it's got one problem that I'm having trouble working around.

@embano1
Copy link
Collaborator

embano1 commented Jul 21, 2024

Looks good to me. I wonder how I managed to not notice atomic.Pointer.

They were introduced somewhat "recently" in Go 1.19, and atomic.Value has been around for longer (and during the time you created Quamina) - so there's no-one to blame here ;)

+1 on using atomic.Pointer

@timbray timbray merged commit 8da5067 into timbray:main Jul 21, 2024
7 checks passed
@timbray
Copy link
Owner

timbray commented Jul 22, 2024

Hey @arnehormann I have an overall performance test script and the change to atomic.Pointer led to an 0.52% improvement overall and 1.37% in the most common case.

@arnehormann
Copy link
Collaborator Author

That sounds wonderful! Is have hoped for more, but it's a lot for such a small change. Also, live by a thousand paper-mends or somesuch

@arnehormann arnehormann deleted the atomic.Value-to-Pointer branch October 28, 2024 19:54
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.

4 participants