Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ReferenceCountUtil.touch(...) calls before we store messages into… #13838

Merged
merged 4 commits into from
Feb 12, 2024

Conversation

normanmaurer
Copy link
Member

… a queue to make debugging leaks easier

Motivation:

We should touch our messages when we take over ownership, this allows easier debugging of buffer leaks.

Modifications:

Let's add touch call before enqueue into internal datastructure

Result:

Easier to debug buffer leaks

… a queue to make debugging leaks easier

Motivation:

We should touch our messages when we take over ownership, this allows easier debugging of buffer leaks.

Modifications:

Let's add touch call before enqueue into internal datastructure

Result:

Easier to debug buffer leaks
@franz1981
Copy link
Contributor

Why not calling buf.touch directly? It would resolve to a nice invokevirtual instead of interface check and invokeinterface

@normanmaurer
Copy link
Member Author

Why not calling buf.touch directly? It would resolve to a nice invokevirtual instead of interface check and invokeinterface

done

@normanmaurer
Copy link
Member Author

@franz1981 PTAL again

@franz1981
Copy link
Contributor

franz1981 commented Feb 12, 2024

@normanmaurer

I see there are still touch around; remember #13782 where I've introduced an inlined concrete class check because is in the hot path there?
Given that the paths where is added now are super-hot, I really suggest to make it obvious to the JIT which method we want to call, doing something similar.

I know it's sad we have to perform something like that, but it depends by how many "receivers" (ie different concrete classes) are observed at the instanceof call-site there.
If there is more than one (UnpooledXY, FileRegion, PooledZW) than the JIT has to perform the infamous interface check.
Moreover, invokeinterface right after, has to perform a O(n) scan itself to find the right interface method offset to call it (this instead happens only if there are more than 2 receivers, instead, AFAIK): I've refreshed my knowledge on it at https://blogs.oracle.com/javamagazine/post/mastering-the-mechanics-of-java-method-invocation.

@normanmaurer
Copy link
Member Author

@franz1981

@normanmaurer

I see there are still touch around; remember #13782 where I've introduced an inlined concrete class check because is in the hot path there? Given that the paths where is added now are super-hot, I really suggest to make it obvious to the JIT which method we want to call, doing something similar.

I know it's sad we have to perform something like that, but it depends by how many "receivers" (ie different concrete classes) are observed at the instanceof call-site there. If there is more than one (UnpooledXY, FileRegion, PooledZW) than the JIT has to perform the infamous interface check. Moreover, invokeinterface right after, has to perform a O(n) scan itself to find the right interface method offset to call it (this instead happens only if there are more than 2 receivers, instead, AFAIK): I've refreshed my knowledge on it at https://blogs.oracle.com/javamagazine/post/mastering-the-mechanics-of-java-method-invocation.

Yeah I hink you are right... PTAL again.

Copy link
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

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

And thanks for the patience @normanmaurer , much appreciated!

@normanmaurer normanmaurer merged commit 8d7c113 into 4.1 Feb 12, 2024
12 of 13 checks passed
@normanmaurer normanmaurer deleted the touch branch February 12, 2024 18:04
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.

None yet

3 participants