-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Replace sun.misc.Unsafe used in LazyVals with VarHandles #24109
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
Conversation
a82a297
to
5aafc89
Compare
I actually wonder: is that worth it? It seems to bring additional complexity to the transformation. Sure, we need one call for every
IMO that is really worth it, but for a different reason: it removes the last reference to the (We can drop the |
It's an obvious improvement so I wanted to take it. Unfortunately, even without this we still had to introduce complexity in the MoveStatics phase (directly linking to the LazyVals phase), so we are not doing that much more (1st commit vs 2nd commit). I'll try running some benchmark tests to see if it's worth it.
Right, I hadn't even thought about that (we still reference the nested classes but now I realize that's not an issue). Weirdly, even with the objCAS2 call, the warnings disappeared for me. |
9032701
to
bb1b2ca
Compare
@sjrd Issues with CI took me a little more time than I anticipated, so I haven't done the promised benchmarks yet. Still I am willing to remove the initialization optimization if it complicates things too much. I did test whether this works with native image though (it does). |
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.
Nice work overall!
private lazy val minTargetVersion = classfileVersionMap.keysIterator.min | ||
private lazy val maxTargetVersion = classfileVersionMap.keysIterator.max | ||
|
||
private val minReleaseVersion = 17 |
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.
Shouldn't we change classfileVersionMap
instead?
(minTargetVersion to maxTargetVersion).toList.map(_.toString) | ||
|
||
def supportedReleaseVersions: List[String] = | ||
if scala.util.Properties.isJavaAtLeast("9") then |
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.
This is true
now.
val varHandleInfo = appendVarHandleDefs.getOrElseUpdate(claz, new VarHandleInfo(EmptyTree, Nil)) | ||
varHandleInfo.methodHandlesLookupDef match | ||
case EmptyTree => | ||
val lookupSym: TermSymbol = newSymbol(claz, (s"${claz.name}${lazyNme.methodHandleLookupSuffix}").toTermName, Synthetic, defn.MethodHandlesLookupClass.typeRef).enteredAfter(this) |
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.
Turning name
s into strings like that is not recommended. Consider something like
val lookupSym: TermSymbol = newSymbol(claz, (s"${claz.name}${lazyNme.methodHandleLookupSuffix}").toTermName, Synthetic, defn.MethodHandlesLookupClass.typeRef).enteredAfter(this) | |
val lookupSym: TermSymbol = newSymbol(claz, claz.name.toTermName.withSuffix(lazyNme.methodHandleLookupSuffix), Synthetic, defn.MethodHandlesLookupClass.typeRef).enteredAfter(this) |
case _ => | ||
|
||
// create a VarHandle for this lazy val | ||
val varHandleSymbol: TermSymbol = newSymbol(claz, s"$containerName${lazyNme.lzyHandleSuffix}".toTermName, Synthetic, defn.VarHandleClass.typeRef).enteredAfter(this) |
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.
Same here:
val varHandleSymbol: TermSymbol = newSymbol(claz, s"$containerName${lazyNme.lzyHandleSuffix}".toTermName, Synthetic, defn.VarHandleClass.typeRef).enteredAfter(this) | |
val varHandleSymbol: TermSymbol = newSymbol(claz, containerName.toTermName.withSuffix(lazyNme.lzyHandleSuffix), Synthetic, defn.VarHandleClass.typeRef).enteredAfter(this) |
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.
Also, can't we make those symbols Private
?
} | ||
|
||
extension (sym: Symbol) def isVarHandleForLazy(using Context) = | ||
sym.name.endsWith(lazyNme.lzyHandleSuffix) |
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.
Looks like we should use a dedicated NameKind
if we need to do that kind of test.
|
||
val staticAssigns = staticFields.map(x => Assign(ref(x.symbol), x.rhs.changeOwner(x.symbol, staticCostructor))) | ||
tpd.DefDef(staticCostructor, Block(staticAssigns, tpd.unitLiteral)) :: newBody | ||
val symbolRemap = localStaticDefs.map { x => |
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.
IMO this is too complex to introduce without a very good reason.
I suggest we stick to the simpler alternative where we repeat the calls to lookup()
.
localStaticDefs.addOne(valDef) | ||
case memberDef: MemberDef if memberDef.symbol.isScalaStatic => | ||
if memberDef.symbol.isVarHandleForLazy then | ||
staticTiedDefs.addOne(memberDef) |
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.
It looks like this could be
staticTiedDefs.addOne(memberDef) | |
remainingDefs.addOne(memberDef) |
which would remove the need for staticTiedDefs
altogether.
import dotty.tools.pc.utils.JRE | ||
|
||
class CompletionRelease8Suite extends BaseCompletionSuite: | ||
@Ignore class CompletionRelease8Suite extends BaseCompletionSuite: |
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.
Looks like this still needs to be addressed?
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.
It doesn't work anymore, because the -release 8 option throws an error now (on purpose). I'll add the comment explaining that. If it's clearer to just remove the test completely I can do that too
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.
I see. Yes, in that case we should remove the test entirely.
Perhaps extract a PR that really gets rid of JDK 8 support. There seem to be several commits about that in this PR, and they should stand on their own.
lazy val i18231 = project | ||
.in(file("i18231")) | ||
.settings(scalacOptions += "-release:8") | ||
// .settings(scalacOptions += "-release:8") |
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.
Doesn't that invalidate the purpose of the test? Should it be retargeted to 17?
This is paving the way for #24109. As part of this: * the minimum version for `-release`/`-java-output-version` flag is set to 17 * managed community build tests are done on JDK 17 * some community-build projects are temporarily removed * some that were using -release 8 option now have that option removed * compilation tests using `-release 8` were disabled * presentation compiler tests using `-release 8` and `-release 11` were removed * releases are done on JDK 17
c578c64
to
045cec4
Compare
045cec4
to
aa26211
Compare
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.
Only an optional nit. LGTM. Nice improvement, even. :)
val varHandleSymbol: TermSymbol = newSymbol(claz, LazyVarHandleName(containerName), Private | Synthetic, defn.VarHandleClass.typeRef).enteredAfter(this) | ||
varHandleSymbol.addAnnotation(Annotation(defn.ScalaStaticAnnot, varHandleSymbol.span)) | ||
val getVarHandle = Apply( | ||
Select(Apply(Select(ref(defn.MethodHandlesClass), defn.MethodHandles_lookup.name), Nil), defn.MethodHandlesLookup_FindVarHandle.name), |
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.
Directly select the symbols?
Select(Apply(Select(ref(defn.MethodHandlesClass), defn.MethodHandles_lookup.name), Nil), defn.MethodHandlesLookup_FindVarHandle.name), | |
Select(Apply(Select(ref(defn.MethodHandlesClass), defn.MethodHandles_lookup), Nil), defn.MethodHandlesLookup_FindVarHandle), |
or maybe it's
Select(Apply(Select(ref(defn.MethodHandlesClass), defn.MethodHandles_lookup.name), Nil), defn.MethodHandlesLookup_FindVarHandle.name), | |
Apply(ref(defn.MethodHandlesClass).select(defn.MethodHandles_lookup), Nil).select(defn.MethodHandlesLookup_FindVarHandle), |
Thank you for the reviews and support @sjrd! |
Btw. this will not be backported to LTS, which means LTS will get the warnings about Unsafe deprecation and might eventually not work on a newer Java version. That said, it's encouraged if you want to work on newest JDK to just use Scala Next Great work @jchyb ! |
@tgodzik Couldn't this be an issue for Libs authors, who will want to provide official support to JDK25 but can't use Scala Next to allow everyone to use the Scala version they want? |
Might be, though if someone is using JDK 25 for the library, they are already forcing users pretty heavily with the JDK bump. Do you know any such library? I assumed that most libraries would release on max 21. Library authors were usually quite careful about that. |
From https://openjdk.org/jeps/471:
and
Does this mean that library built with latest Scala 3 LTS might not be usable on JDK 26+? |
That is possible, but we can't change the implementation of lazy vals within a minor. There will be a new LTS soon in the 3.9.x line. As a side note we should probably make sure that this is detected during compilation. |
I did the very quick and uneducated skim of changes, but I didn't notice public API changes (except those few ignored ones). Isn't implementation of lazy vals private? |
When I say "providing official JDK 25 support", it's just running the CI with JDK 25 (additionally with 17 and 21, with a minimum required version of 17). Most libs will adopt this strategy. Hence, my question: will it be an issue not to backport this to Scala 3.3.x? Or will it work with just some warnings from JDK 25? |
For JDK 25 it will only show warnings now, so we have some time to decide if the situation requires for us to try and backport this at some point anyway. |
I can't see how we could back port this to Scala 3.3.x, as the new API uses JDK API which is absent in Java 8. Library authors wanting to run on JDK 25 will have to bump to Scala 3.9 LTS, which is right behind the corner. |
Thanks for your clarifications 🙏 |
Scala 2.13 does exactly that:
performs one-time feature discovery on JVM:
as a result all code compiled on 2.13.7+, should work on newer JDKs. With no recompilation. Since 7 years ago. It means that currently libraries' authors would have less issues supporting 2.13 for both newer and older JDK than Scala 3 authors - on 2.13 code just works, on 3 I will have yet another issue to solve when it comes to supporting Scala 2.13 + Scala 3. |
What Scala 2.13 does here has nothing to do with For Scala 2.13 does not have the |
I'd argue that it's not possible to do it in exactly the same way, but still possible. OTOH ideas:
For me, the question is more about what utilities can and cannot be added to LTS (if they are not directly exposed to the users), and if none (because someone can still downgrade the patch version), how much extra code can be generated (possibly behind a flag, due to a probable performance hit), rather than whether it's possible or not. |
Both of your solutions would be disastrous for performance. They're not viable in this context. |
Another possibility is to allow using I prefer any such option to dropping support in my libraries. |
And, during compilation, |
Targeting something newer than "8" is mostly irrelevant - any of your dependencies can be compiled with a version of the compiler that generated code using I think the idea that 3.9 is the LTS that works with JDK 26 as a clean cut off barrier is actually a good choice, because:
It is an uncomfortable situation but the only way this could have been prevented was to drop JDK 8 before and that would make a lot of people very unhappy anyway. |
Scala 3.3 LTS will be supported for at least a year after 3.9 LTS is out. The way things are, it does not seem to me that JDK 26+ would be properly supported on versions older than the upcoming 3.9 LTS series.
A warning already is being printed on JDK 25. scala -e 'println("Hello")' -S 3.3.7--jvm 25
# Compiling project (Scala 3.3.7, JVM (25))
# Compiled project (Scala 3.3.7, JVM (25))
# WARNING: A terminally deprecated method in sun.misc.Unsafe has been called
# WARNING: sun.misc.Unsafe::objectFieldOffset has been called by scala.runtime.LazyVals$ (file:~/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala3-library_3/3.3.7/scala3-library_3-3.3.7.jar)
# WARNING: Please consider reporting this to the maintainers of class scala.runtime.LazyVals$
# WARNING: sun.misc.Unsafe::objectFieldOffset will be removed in a future release
# Hello Or do you mean that we should warn about JDK 25 limitations while targeting JDK 8? If so, I am lost here. |
The valid issue that @MateuszKubuszok is raising here is that it is going to be a problem for library maintainers that want to be able to release new releases for people on older JDKs. We already have a matrix because of 2.12, 2.13 and 3 and the promise was that 3.x would just work without any splits so once a lib drops 2.x, it's free of any versioning matrices altogether. @MateuszKubuszok's libs were hit with a backwards incompatible change when implicit resolution rules changed and it was a difficult problem to fix. @jchyb was involved with design and implementation of a new macro API that allowed to deal with that. Now this problem hits even more libs as it does introduce a compatibility matrix problem if you are unwilling to drop support for jdk <17 for your users by just bumping required Scala version to 3.9 for 3.x branch. So we will, indeed, have two 3.x variants for such maintainers - 3.3 with the jdk8-jdk25 range and 3.9 with the jdk17-jdk26+ range. I do understand the frustration but there's very little that can be done if we are to treat backwards compatibility in LTS seriously. |
Closes #9013
Just a few small tweaks left:[x] maybe inline objCAS2 (should give us more freedom, allow us to backport into 3.3 in the worst case scenario (hopefully not))EDIT: done
This PR replaces the use of the deprecated Unsafe with VarHandles, available in java 9+. Important thing about VarHandles is that for private members (like our val dictating the value/resolution progress of the lazy val), the findVarHandle method must be called from the class containing the val, otherwise we end up with an error. This meant that some custom logic had to be added to the MoveStatics phase, which would usually move static constructor to the companion object (which we do not want for the VarHandles here).
Only the newer implementation was adjusted, the one available under
-Ylegacy-lazy-vals
was left untouched.This PR also effectively drops support for java 8, with java 17 being the new required version. As part of that:-release
/-java-target-version
is now 17 (earlier versions will throw an error). -Xunchecked-java-output-version was left untouched.-release 8
were removed for projects that were using that. Projects that were usingjavax.xml.bind
(dropped in java 11) were removed altogether (playJson and betterfiles)Java version used for releases was set to 17(Added separately in Require JDK 17+ #24146)