Skip to content

Make ActionListener.notifyOnce a little cheaper#97361

Merged
original-brownbear merged 4 commits intoelastic:mainfrom
original-brownbear:cleanup-notify-once
Jul 10, 2023
Merged

Make ActionListener.notifyOnce a little cheaper#97361
original-brownbear merged 4 commits intoelastic:mainfrom
original-brownbear:cleanup-notify-once

Conversation

@original-brownbear
Copy link
Copy Markdown
Contributor

This is used on the rather hot path now due to #97301, lets apply the optimization of saving one level of indirection here as well to make GC etc. a little cheaper on these.

This is used on the rather hot path now due to #97301, lets apply the optimization
of saving one level of indirection here as well to make GC etc. a little cheaper on these.
@original-brownbear original-brownbear added >non-issue :Core/Infra/Core Core issues without another label labels Jul 4, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 4, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Hm subclassing AtomicReference feels like a bit of a hack to me, I'd kinda prefer us to use a VarHandle directly. If you have strong reasons not to do so, they deserve a comment.

@original-brownbear
Copy link
Copy Markdown
Contributor Author

@DaveCTurner it's just less code and we have the thing package private here now anyway, so why blow up the code size when we won't be leaking that implementation detail? (we did the same thing elsewhere for private classes like ReleaseOnce)

@DaveCTurner
Copy link
Copy Markdown
Member

Yeah I have the same feeling about ReleaseOnce 😁

I'd rather be explicit even if it is a few more lines of code - these things are a little subtle already.

Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM (for some value of "G" at least 😉)

@original-brownbear
Copy link
Copy Markdown
Contributor Author

Thanks David :)

@original-brownbear original-brownbear merged commit bb7dd06 into elastic:main Jul 10, 2023
@original-brownbear original-brownbear deleted the cleanup-notify-once branch July 10, 2023 09:25
@original-brownbear original-brownbear restored the cleanup-notify-once branch November 8, 2023 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.10.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants