ARTEMIS-6289 Use AtomicLong instead of synchronized counter in SimpleIDGenerator by amarkevich · Pull Request #6770 · apache/artemis

amarkevich · 2026-10-06T13:09:05Z

No description provided.

jbertram · 2026-10-06T15:11:36Z

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).

clebertsuconic · 2026-10-06T15:16:49Z

+      return idSequence.getAndUpdate(id -> {
+         if (id == Long.MAX_VALUE) {
+            // Wrap - Very unlikely to happen
+            throw new IllegalStateException("Exhausted ids to use!");


I would set it to 0 if this ever happens... similar to what the Journal backed generator does.

The constructor does not enforce that the sequence starts at/above 0 so it could as equally have been Long.MIN_VALUE it started at, or any other value. Wrapping to 0 would seem to be making an assumption which doesnt necessarily fit with that constructor?

I just meant to wrap like it used to be. it used to be MIN_VALUE I guess.

It didnt used to wrap at all, it threw which is why I was confused between this and the other comment (which I wonder if you meant to reply to here instead) that seemed they might conflict.

Well ok...it wrapped the field, noticed, remembered it, then threw rather than return any value. Is that what you meant? Here it just noticed the value (tracing the current/previous id) hit MAX_VALUE and threw before the incrementing wrapped, so that it didnt need to remember anything.

clebertsuconic · 2026-10-06T15:17:52Z

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


I would keep the previous semantics.

I'm not clear which semantics you mean here, can you clarify? Especially as you have 2 comments from <1min apart overlapping 3 of the same line numbers of code context.

tabish121 · 2026-10-06T15:27:50Z

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 · 2026-10-06T16:14:15Z

...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 · 2026-10-06T16:16:18Z

...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 · 2026-10-06T16:42:47Z

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.

gemmellr · 2026-10-07T14:47:39Z

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.

There seem like enough connection and session specific uses of it that it might be worth it in this case? Having one would mean both use cases were covered, vs having both implementations as the PR now seems to?

gemmellr · 2026-10-07T14:51:45Z

+   /**
+    * {@link AtomicLongFieldUpdater} can only bind to a {@code volatile} <em>instance</em> field
+    * ({@code newUpdater} throws {@link IllegalArgumentException} for static fields), so the
+    * JVM-wide counter lives on this single static holder instance.


Why is this generator now being made JVM-wide? It doesnt seem like it was originally in the place you used it, but rather per-session.

Unclear why this was resolved...unresolving.

move logic completely to ActiveMQSessionContext to avoid generator instance creation

I'm not sure if you arent understanding my question or are just repeatedly ignoring it.

Why have you changed the semantics of the session-level consumer id, from being indexed per-session to being per-JVM? I see nothing here or the Jira about why that is being done.

got your point - reverted per-session counter

SimpleIDGenerator

clebertsuconic reviewed Oct 6, 2026

View reviewed changes

amarkevich force-pushed the ARTEMIS-6289 branch from 1d89e67 to 4aa20c3 Compare October 6, 2026 16:27

amarkevich force-pushed the ARTEMIS-6289 branch from 4aa20c3 to 43babc8 Compare October 7, 2026 13:04

amarkevich changed the title ~~ARTEMIS-6289 Use AtomicLong instead of synchronized counter in SimpleIDGenerator~~ ARTEMIS-6289 Use AtomicLong instead of synchronized counter in SimpleIDGenerator; introduce SharedIDGenerator Oct 7, 2026

gemmellr reviewed Oct 7, 2026

View reviewed changes

Comment thread artemis-core-client/src/test/java/org/apache/activemq/artemis/utils/SharedIDGeneratorTest.java Outdated

gemmellr reviewed Oct 7, 2026

View reviewed changes

amarkevich force-pushed the ARTEMIS-6289 branch 2 times, most recently from 1ed70f1 to 869ade2 Compare October 8, 2026 08:27

amarkevich changed the title ~~ARTEMIS-6289 Use AtomicLong instead of synchronized counter in SimpleIDGenerator; introduce SharedIDGenerator~~ ARTEMIS-6289 Use AtomicLong instead of synchronized counter in SimpleIDGenerator; AtomicLongFieldUpdater for ActiveMQSessionContext Oct 8, 2026

amarkevich force-pushed the ARTEMIS-6289 branch from 869ade2 to dc6917e Compare October 8, 2026 11:03

ARTEMIS-6289 Use AtomicLong instead of synchronized counter in

3e19475

SimpleIDGenerator

amarkevich force-pushed the ARTEMIS-6289 branch from dc6917e to 3e19475 Compare October 14, 2026 15:25

amarkevich changed the title ~~ARTEMIS-6289 Use AtomicLong instead of synchronized counter in SimpleIDGenerator; AtomicLongFieldUpdater for ActiveMQSessionContext~~ ARTEMIS-6289 Use AtomicLong instead of synchronized counter in SimpleIDGenerator Oct 14, 2026

Conversation

amarkevich commented Oct 6, 2026

Uh oh!

jbertram commented Oct 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemmellr Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tabish121 commented Oct 6, 2026

Uh oh!

jbertram commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tabish121 commented Oct 6, 2026

Uh oh!

jbertram commented Oct 6, 2026

Uh oh!

Uh oh!

gemmellr commented Oct 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gemmellr Oct 7, 2026 •

edited

Loading

jbertram commented Oct 6, 2026 •

edited

Loading