Skip to content

ARTEMIS-6289 Use AtomicLong instead of synchronized counter in SimpleIDGenerator - #6770

Open
amarkevich wants to merge 1 commit into
apache:mainfrom
amarkevich:ARTEMIS-6289
Open

amarkevich wants to merge 1 commit into
apache:mainfrom
amarkevich:ARTEMIS-6289

Conversation

@amarkevich

Copy link
Copy Markdown
Contributor

No description provided.

@jbertram

jbertram commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

This looks good to me, although org.apache.activemq.artemis.utils.SimpleIDGenerator doesn't get a lot of (if any) concurrent use. @clebertsuconic, @gemmellr, what do you think?

That said, I did notice that there's no unit test for org.apache.activemq.artemis.utils.SimpleIDGenerator at all. It might be nice to add one (e.g. to ensure IllegalStateException is thrown as expected).

public long generateID() {
return idSequence.getAndUpdate(id -> {
if (id == Long.MAX_VALUE) {
// Wrap - Very unlikely to happen

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would keep the previous semantics.

@tabish121

Copy link
Copy Markdown
Contributor

If making changes here it would make more sense to use AtomicLongFieldUpdater and a single volatile long value instead of creating a new AtomicLong for every instance of this type.

@jbertram

jbertram commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

...it would make more sense to use AtomicLongFieldUpdater and a single volatile long value...

I'm not sure there would ever be enough instances of SimpleIDGenerator on the broker to justify this.

@tabish121

Copy link
Copy Markdown
Contributor

...it would make more sense to use AtomicLongFieldUpdater and a single volatile long value...

I'm not sure there would ever be enough instances of SimpleIDGenerator on the broker to justify this.

I'm not sure why one needs to justify good coding practices...

@jbertram

jbertram commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

I'm not sure why one needs to justify good coding practices...

Perhaps I'm wrong about this, but my understanding is that the main use-case for any field updater is lowering memory use at scale. The main trade-offs being some clunky boilerplate code, the loss of compile-time type safety, and slightly higher per-call overhead. However, if the scale is too low to reap meaningful memory benefits then it seems reasonable to use the standard Atomic* version. I think the latter is true in this case.

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