Skip to content
This repository was archived by the owner on Jan 14, 2026. It is now read-only.

Remove unnecessary AtomicReference#110

Open
spkrka wants to merge 1 commit intomasterfrom
krka/atomic-ref
Open

Remove unnecessary AtomicReference#110
spkrka wants to merge 1 commit intomasterfrom
krka/atomic-ref

Conversation

@spkrka
Copy link
Copy Markdown
Member

@spkrka spkrka commented Jun 4, 2021

No description provided.

Copy link
Copy Markdown
Contributor

@ao2017 ao2017 left a comment

Choose a reason for hiding this comment

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

I am not sure this implementation will work. Look at line 65. distRef is not completely protected. I prefer to keep this implementation till we get a lock free one.

@spkrka spkrka force-pushed the krka/atomic-ref branch from f48f94e to 2e1e2a2 Compare July 15, 2021 09:08
@spkrka spkrka force-pushed the krka/atomic-ref branch from 2e1e2a2 to 35e7187 Compare July 15, 2021 10:01

private static final int COMPRESSION_DEFAULT_LEVEL = 100;
private final AtomicReference<TDigest> distRef;
private volatile TDigest dist;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since dist is covered by synchronized everywhere, does it really need to be a volatile?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Line 85 references dist without being synchronized, so the read might see a stale copy

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd make it synchronized as well instead (like this e.g.).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants