-
Notifications
You must be signed in to change notification settings - Fork 381
Fix #4209: implement j.l.ClassValue #4211
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
Fix #4209: implement j.l.ClassValue #4211
Conversation
| private val cvMap: ConcurrentMap[Class[_], Optional[T]] = | ||
| new ConcurrentHashMap[Class[_], Optional[T]](64) // a guess > default 16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! While this implementation will work, unfortunately in practice it is not very efficient. It's better to have a ClassValue -> T map inside of Class.
You can see some discussion here (no code):
- https://bugs.openjdk.org/browse/JDK-7030453
- https://mail.openjdk.org/pipermail/mlvm-dev/2011-September/003930.html
This bug is mainly a performance bug, due to the use of a global table (keyed by Class) instead of a direct link (from a non-public field in Class).
There is a package-private change to java.lang.Class, adding two new fields (to the existing 19 fields declared in Class.java).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arman,
Thank you for looking at this PR.
All code must perform reasonably. Reading between the lines, it sounds like
you want to use this class on a performance critical path. True?
I read the two supplied openjdk references and the cats-effect reference in the
base Issue.
I am going to have to give this a few days pause and to study _Class.scala .
The stick parts is likely to be not just adding fields but the complicated
concurrency and entry versioning to manage lookups.
Those are the kinds of changes which benefit from a long field test.
What you are implying goes way beyond a simple (sic) concurrent key/value lookup
where a missing value is computed.
I also need to understand ClassValue,its use, and the intended mapping better.
I do not have home field advantage there (or even residence in the same
geographical area).
When you say " It's better to have a ClassValue -> T map inside of Class",
do you mean the Class companion object (if there is such, SN does some
trickery around _Class getting generated as Class in the .ll files.)
I saw and studied the `entry versioning` section in the JVM documentation. When I thought about how I would implement it, I was put off by the fact that at the rate of one change per second a Long version number would overflow in approx 3 trillion years and there was no obvious fallback. Using a BigInteger seemed excessive (and slow).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does cats-effect run on Scala.js? If yes, how did the ClassValue implementation
do there? This one should be slightly faster, other things being equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading between the lines, it sounds like
you want to use this class on a performance critical path. True?
Yes, that's correct.
When you say " It's better to have a ClassValue -> T map inside of Class",
do you mean theClasscompanion object (if there is such, SN does some
trickery around _Class getting generated as Class in the .ll files.)
I meant inside of the Class instance (not the companion).
Conceptually, you can think of Class and ClassValue[T] as comparable to Thread and ThreadLocal[T]. Instead of keeping a Thread -> T map inside of a ThreadLocal, we keep a ThreadLocal -> T map inside of Thread.
scala-native/javalib/src/main/scala/java/lang/Thread.scala
Lines 34 to 36 in d37d9ab
| // ThreadLocal values : local and inheritable | |
| private[java] var threadLocals: ThreadLocal.Values = _ | |
| private[java] var inheritableThreadLocals: ThreadLocal.Values = _ |
Does cats-effect run on Scala.js? If yes, how did the
ClassValueimplementation
do there?
Cats Effect does run on Scala.js. We are still in the process of benchmarking ClassValue on the JVM and JS.
This one should be slightly faster, other things being equal.
Not necessarily. Scala.js doesn't have to worry about multi-threading, which simplifies the implementation (no locking/synchronization).
In any case, I think it's likely that the Scala.js implementation will need a similar refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above is helpful. I'll need to study it and existing SN Threadmumble code next session
(tomorrow and rest of week are booked).
-
When you say
we keep a ThreadLocal -> T mapI take you to mean a "mapping" or translation
not some form of Java Map class? -
Given that we are looking at this after 14+ years of Java development and improvement,
I suspect that some form of compare-and-set (CAS), with retry on error would be better
than "entry versioning". Have to design for the long haul.CAS-with-retry is a delicate idiom but at least well described and with plenty of models.
Time, next session, for some design studies. Now that we have defined both the
key and value as (possibly null) AnyRef, implementation should be easier.I do not want to even think about the null key case or the double null key/value cases.
Arrgh!Is this likely to be worthwhile at the other end?
-
Not asking for a thesis, just a brief description. How are you measuring the performance
of theClassValue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceptually, you can think of Class and ClassValue[T] as comparable to Thread and ThreadLocal[T]. Instead of keeping a Thread -> T map inside of a ThreadLocal, we keep a ThreadLocal -> T map inside of Thread.
Let me try to sketch the transitions out. If I had known this was going to get harder, I would
have eaten a better breakfast and packed a supper.
abstract class ClassValue[T <: AnyRef] protected ()
tells you that T is an AnyRef. The new proposed instance field, Class.mumble, would have
to be an AnyRef.
def get(`type`: Class[_]): T =
The argument "type" gives you a Class, so once can, ignoring synchronization and re-try access variable tpe.mumble
to retrieve an AnyRef which one casts to a T? How does one know that some other thread/process has
not put an AnyRef with an incompatible type into that field so the "asInstanceOf[T]" fails?
// -------
Later: 2025-02-10 18:30 UTC-5
In my waiting-for-other-things-to-happen time I am slowly puzzling things out, but have not
fully unraveled it yet.
ClassValue.computeValue(): T returns a T. There is a verbal description/constraint
that ClassValue.get() stores that returned value-of-T (in the current sketch, in the Class.mumble
field). The Object stored might be something which can be converted to T or from which T can
be fetched. If ClassValue.get() does not follow the verbal contraint and actually stuffs an
anti-T, the consequence is that ClassValue.get() will fail at runtime because the anti-T
can not be converted to a T. That is, it hurts only a future self, if any.
Does that sound like a plausible scenario & justification for lack of "constraint by defined type"?
I have some more waiting around to do tomorrow. I need to start thinking about concurrency
and if "entry versioning" can be avoided. I'd rather deal with trying to update one field
atomically then two (or more), even when AtomicReference is available.
// ------
Later: 2025-02-10 19:35 UTC-5
Like a dog with a bone...
-
I believe that I have convinced myself that "entry versioning" is indeed necessary since
thecomputeValue()method is allowed to maintain internal state. That method is
abstract and outside the control of javalib. -
I found the JDK doc for
AtomicStampedReference. Somebody else must have
faced a similar need. Joy of joys, that class is already implemented in Scala Native.
I think I have also figured out how to initialize it and manage the stamp so
that it wraps around.I need to do a sandbox design study to see if these possibilities pan out.
IfClassValue.get()does not do too much messing around, this looks like
it will be very fast, even under contention. -
I think the current (last?) blocker is the increased memory usage per every
Class instance. I know that the SN optimizer can optimize away unused qua
unreachable methods. Can it detect if an instancevarhas been touched
after it has been initialized and, if not, optimize it away? That is, ifClassValue
is never used, would every Class instance still pay the aligned 8 byte penalty?Thoughts? Obviously the answer to that is "get better/bigger bones".
// ------
Is the second proposed new Class field the original type of the AnyRef? Both must be fetched
together, at once and the fetched type must match T?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the proposal is that every Class instance becomes 8 or 16 bytes larger, even
if ClassValue is never used? Seems like a tax or unfunded mandate.
|
Marking as draft whilst I work on an alternative. Then we can have a bake-off. |
|
To keep a record of discussions widely scattered in time and across the Web: One of the discussed purported disadvantages of the HashMap approach is that Editorial comment: |
|
The helpful discussion in Issue #4209 leads to to closing this. |
Fix #4209
Port javalib
java.lang.ClassValueand itsClassValueTestfrom Scala.js, with thanks & appreciation.Significantly modified both for Scala Native.