Skip to content

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 force-pushed the atomic.Value-to-Pointer branch from bf5a457 to 84f7abb Compare July 20, 2024 11:40
@arnehormann arnehormann changed the title prefer atomic.Pointer over atomic.Value prefer atomic.Pointer to atomic.Value Jul 20, 2024
@arnehormann arnehormann force-pushed the atomic.Value-to-Pointer branch from 84f7abb to e8ba530 Compare July 20, 2024 13:26
@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
@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