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

Use AtomicLongFieldUpdater in StepLong, StepDouble, and AtomicDouble #1158

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

kilink
Copy link
Member

@kilink kilink commented Sep 14, 2024

Use AtomicLongFieldUpdater to shrink the footprint of StepLong, StepDouble, and AtomicDouble.

From a heapdump for a typical app, we see tens of thousands of instances of these classes:

StepLong: 21,751 instances
StepDouble: 29,233 instances
AtomicDouble 31,822 instances

Getting rid of the object reference to an AtomicLong shrinks the allocation footprint and avoids the extra pointer chasing. Note that StepLong and StepDouble still have a field current that is an AtomicLong / AtomicDouble; it can't be inlined into a volatile field without breaking backwards compatibility since it is exposed via the getCurrent method.

@@ -124,4 +127,9 @@ public void max(double v) {
@Override public double doubleValue() {
return get();
}

@Override
public String toString() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a toString implementation to match the behavior of AtomicLong / AtomicInteger

@kilink kilink force-pushed the atomic-field-updaters branch 2 times, most recently from 1ce7e11 to e1fdb84 Compare September 14, 2024 21:42
Use AtomicLongFieldUpdater to shrink the footprint of StepLong, StepDouble, and AtomicDouble.
@kilink kilink force-pushed the atomic-field-updaters branch from e1fdb84 to 645a878 Compare September 14, 2024 21:43
@kilink kilink requested a review from brharrington September 14, 2024 21:44
@brharrington brharrington added this to the 1.8.0 milestone Sep 15, 2024
@brharrington
Copy link
Contributor

Will need to do some testing on this, probably won't have time this week.

@brharrington
Copy link
Contributor

Note that StepLong and StepDouble still have a field current that is an AtomicLong / AtomicDouble; it can't be inlined into a volatile field without breaking backwards compatibility since it is exposed via the getCurrent method.

Since the impl package is for implementation details and should only be used within spectator itself, it should be ok for those to get updated. Users should always have aligned versions of spectator sub-projects and internally that is enforced via resolution rules.

The common operations are now available directly on the
StepLong/StepDouble.
Copy link
Contributor

@brharrington brharrington left a comment

Choose a reason for hiding this comment

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

I think this should be good. Throughput reduction seems fairly minimal. Do you mind doing a sanity check of the commit I pushed to use the updater pattern for the current field on StepLong and StepDouble?

@kilink
Copy link
Member Author

kilink commented Oct 11, 2024

I think this should be good. Throughput reduction seems fairly minimal. Do you mind doing a sanity check of the commit I pushed to use the updater pattern for the current field on StepLong and StepDouble?

Looks good to me.

@brharrington brharrington merged commit 17e6d74 into Netflix:main Oct 11, 2024
1 check passed
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.

2 participants