Skip to content

don't use unsafe to update nextName#1892

Merged
pjfanning merged 7 commits into
apache:mainfrom
pjfanning:nextname
Aug 1, 2025
Merged

don't use unsafe to update nextName#1892
pjfanning merged 7 commits into
apache:mainfrom
pjfanning:nextname

Conversation

@pjfanning

@pjfanning pjfanning commented Jun 7, 2025

Copy link
Copy Markdown
Member

@pjfanning
pjfanning marked this pull request as draft June 7, 2025 12:17
}

@nowarn @volatile private var _nextNameDoNotCallMeDirectly = 0L
private val _nextName = new java.util.concurrent.AtomicLong()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

better go with AtomicLongFieldUpdater.

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.

I tried AtomicLongFieldUpdater but it doesn't work with private fields.

[error] Caused by: java.lang.IllegalAccessException: class example.AbstractActorCell cannot access a member of class example.ActorCell with modifiers "private volatile"

I also found in benchmarks that AtomicLong is nearly as fast as the existing code and the code is much simpler to maintain. No reflection, no sun.misc.Unsafe.

https://github.com/pjfanning/pekko-bench

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.

I actually got AtomicLongFieldUpdater to work by moving the code to be in the class that has the private field. The performance in my bench testing is no better than the simpler AtomicLong.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AtomicLongFieldUpdater can save memory

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.

AtomicLongFieldUpdater is slower in my testing

@He-Pin

He-Pin commented Jun 8, 2025

Copy link
Copy Markdown
Member

Track upstream: scala/scala3#9013

@He-Pin

He-Pin commented Jun 8, 2025

Copy link
Copy Markdown
Member

User can use --sun-misc-unsafe-memory-access=allow for now

@pjfanning
pjfanning marked this pull request as ready for review June 20, 2025 11:57
}

@nowarn @volatile private var _nextNameDoNotCallMeDirectly = 0L
private val _nextName = new java.util.concurrent.atomic.AtomicLong()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

this article is 12 years old. Java runtimes and their memory allocation have improved a lot since then.

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.

even netty uses AtomicLongs

https://github.com/search?q=repo%3Anetty%2Fnetty%20atomiclong&type=code

It also uses the FieldUpdater in a few places but the Netty team seem to believe that the FieldUpdater doesn't need to be used in every case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's not true, it's still valid, you can test it with jol.

@pjfanning
pjfanning marked this pull request as draft June 20, 2025 17:39
@pjfanning

Copy link
Copy Markdown
Member Author

I've tried a few ways to get AtomicLongFieldUpdater to work but so far no look. AtomicLongFieldUpdater does runtime checks and can only access fields in the same class or public variables - but we can't make the long public because then it could be manipulated by any code and it isn't good to have a AtomicLongFieldUpdater declared in the Children trait because you get one instance per ActorCell. I tried using a companion object but in Java that's a separate class and you can't then access a private field in another class.
It's easier in Java because you can declare static and non-static vars in the same class. It tried creating a Java abstract class that does this and have it inherited by Children trait. This causes ActorCell not to compile.

@pjfanning
pjfanning marked this pull request as ready for review July 31, 2025 20:34
@pjfanning
pjfanning requested a review from mdedetrich July 31, 2025 20:34

@mdedetrich mdedetrich left a comment

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.

Instead of asInstanceOf[Long] use : Long instead i.e.

val num = AbstractActorCell.nextNameHandle.getAndAdd(this, 1L).asInstanceOf[Long]

should be

val num = AbstractActorCell.nextNameHandle.getAndAdd(this, 1L): Long
val num: Long = AbstractActorCell.nextNameHandle.getAndAdd(this, 1L)

Might also work.

The reason is that using val num: Long or : Long on the right hand side of the expression is a type ascription which means it completely avoids a runtime call (as long as the expression satisfies the compiler), see https://www.baeldung.com/scala/type-casting#type-ascriptions

Comment thread actor/src/main/scala/org/apache/pekko/actor/dungeon/Children.scala

@mdedetrich mdedetrich left a comment

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.

lgtm (pending CI pass). Left one additional comment, not critical but it looks a bit sus.

@pjfanning pjfanning added this to the 2.0.x milestone Jul 31, 2025
@He-Pin

He-Pin commented Aug 1, 2025

Copy link
Copy Markdown
Member

Netty recently using varhandle
netty/netty#15486

@He-Pin He-Pin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@pjfanning
pjfanning merged commit 8149625 into apache:main Aug 1, 2025
9 checks passed
@pjfanning
pjfanning deleted the nextname branch August 1, 2025 07:53
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