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

Swapped out static tag approach with ClassValue cache #4266

Open
wants to merge 3 commits into
base: series/3.6.x
Choose a base branch
from

Conversation

djspiewak
Copy link
Member

@djspiewak djspiewak commented Feb 5, 2025

All credit goes to @xuwei-k here. I even cribbed his code more or less directly. I haven't yet run the full set of benchmarks so hold off on merging.

Fixes #4260

@djspiewak djspiewak changed the title Swapped out static tag approach with ClassValue cache (ht @xuwei-k) Swapped out static tag approach with ClassValue cache Feb 5, 2025
@armanbilge armanbilge linked an issue Feb 5, 2025 that may be closed by this pull request
@djspiewak
Copy link
Member Author

Ah, unfortunate discovery: ClassValue doesn't exist on Scala Native!

@armanbilge
Copy link
Member

Darn, unless we do some messy source-splitting, that means we won't get this in until we complete the 0.5 upgrade ... anyway I opened an issue.

@djspiewak
Copy link
Member Author

Yeah I briefly thought about doing some sort of weird split thing but it would be bonkers. So I'll probably just push this far enough to get a full set of benchmark results so we can prioritize down the road, then leave it on ice until SN catches up.

@armanbilge
Copy link
Member

Studied the implementation of ClassValue a bit, it still has some fairly complicated machinery. I wonder if we can beat it by doing something similar and simpler (I will prototype this later when I have time). The one definitive advantage that ClassValue has is that it's implemented by a field on Class.

@djspiewak
Copy link
Member Author

The one definitive advantage that ClassValue has is that it's implemented by a field on Class.

Ostensibly it's also something the JIT understands. I think that's the real advantage, but we would need to actually examine the hotspot outputs to be sure.

@armanbilge
Copy link
Member

I wonder if we can beat it by doing something similar and simpler (I will prototype this later when I have time).

I made an experimental "ClassByteMap" in 1472f3d, seems to be faster than ClassValue:

Benchmark                   Mode  Cnt         Score        Error  Units
TagBenchmark.classByteMap  thrpt   20  10417078.521 ± 466789.955  ops/s
TagBenchmark.classValue    thrpt   20   8920919.389 ± 110746.064  ops/s
TagBenchmark.tag           thrpt   20   7082499.380 ± 132528.920  ops/s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use java.lang.ClassValue instead of IO.tag
2 participants