Add `ClassValueCompat` to support systems without `java.lang.ClassValue` (such as Android) #9752

nwk37011 · 2021-09-07T03:50:54Z

The goal of this commit is to enable systems without java.lang.ClassValue to fall back to other method which does not utilize java.lang.ClassValue while keeping its current behavior as how it is for systems with java.lang.ClassValue. Use of java.lang.ClassValue seems mainly for caching as #7879 introduced, so I ran a related benchmark from one of the commits and got following results

Baseline

[info] Benchmark                                             Mode  Cnt   Score    Error   Units
[info] ClassTagBenchmark.lookupClassTag                      avgt   20  40.328 ±  0.496   ns/op
[info] ClassTagBenchmark.lookupClassTag:·gc.alloc.rate       avgt   20  ≈ 10⁻⁴           MB/sec
[info] ClassTagBenchmark.lookupClassTag:·gc.alloc.rate.norm  avgt   20  ≈ 10⁻⁵             B/op
[info] ClassTagBenchmark.lookupClassTag:·gc.count            avgt   20     ≈ 0           counts

ClassValueCompat - java.lang.ClassValue exists

[info] Benchmark                                             Mode  Cnt   Score    Error   Units
[info] ClassTagBenchmark.lookupClassTag                      avgt   20  42.834 ±  0.231   ns/op
[info] ClassTagBenchmark.lookupClassTag:·gc.alloc.rate       avgt   20  ≈ 10⁻⁴           MB/sec
[info] ClassTagBenchmark.lookupClassTag:·gc.alloc.rate.norm  avgt   20  ≈ 10⁻⁵             B/op
[info] ClassTagBenchmark.lookupClassTag:·gc.count            avgt   20     ≈ 0           counts

For cases where systems lacking java.lang.ClassValue, I earned the following result which seems obviously slower than the above two but still it enables such systems to actually utilize classes such as ClassTag instead of throwing ClassNotFoundException

ClassValueCompat - java.lang.ClassValue does not exist

[info] Benchmark                                                      Mode  Cnt     Score    Error   Units
[info] ClassTagBenchmark.lookupClassTag                               avgt   20    79.452 ±  0.294   ns/op
[info] ClassTagBenchmark.lookupClassTag:·gc.alloc.rate                avgt   20  3838.471 ± 15.265  MB/sec
[info] ClassTagBenchmark.lookupClassTag:·gc.alloc.rate.norm           avgt   20   336.000 ±  0.001    B/op
[info] ClassTagBenchmark.lookupClassTag:·gc.churn.G1_Eden_Space       avgt   20  3843.122 ± 24.785  MB/sec
[info] ClassTagBenchmark.lookupClassTag:·gc.churn.G1_Eden_Space.norm  avgt   20   336.408 ±  1.792    B/op
[info] ClassTagBenchmark.lookupClassTag:·gc.churn.G1_Old_Gen          avgt   20     0.003 ±  0.001  MB/sec
[info] ClassTagBenchmark.lookupClassTag:·gc.churn.G1_Old_Gen.norm     avgt   20    ≈ 10⁻⁴             B/op
[info] ClassTagBenchmark.lookupClassTag:·gc.count                     avgt   20  1525.000           counts
[info] ClassTagBenchmark.lookupClassTag:·gc.time                      avgt   20   655.000               ms

This commit combined with #9739 will open Scala 2.13.x to wider range of systems including Android as discussed and demonstrated at https://users.scala-lang.org/t/scala-2-13-on-android/7235/24

smarter · 2021-09-07T09:13:59Z

Nice! FWIW, I've also opened an issue against Android about the lack of ClassValue, but it's not clear to me if they're interested in fixing it: https://issuetracker.google.com/issues/196063118 (maybe someone with some twitter clout could try bringing attention to this?)

lrytz

Thanks for that contribution, the approach LGTM!

src/library/scala/runtime/ClassValueCompat.java

src/library/scala/compat/ClassValueCompat.scala

src/library/scala/runtime/ModuleSerializationProxy.java

lrytz

I think the rewrite of ModuleSerializationProxy in Scala looks good, but it's certainly not trivial to think of all the possible consequences.

lrytz · 2021-09-30T13:08:18Z

src/library/scala/ClassValueCompat.scala

+
+import scala.util.Try
+
+private[scala] abstract class ClassValueCompat[T] {


Sorry to bring this to discussion again, but do we really want to add this class to the scala package? Even if it's package private it will show up in some places way too prominently. For example when editing a Java file in a mixed project, scala.ClassValueCompat will be a public class.

I'd rather keep it in scala.runtime.

agree; I argued (at least somewhat) against scala.compat, but I think scala is an even worse choice

Issues and fixes mentioned below and here got applied to the latest commit

src/library/scala/ClassValueCompat.scala

src/library/scala/runtime/ModuleSerializationProxy.scala

src/library/scala/runtime/ClassValueCompat.scala

lrytz · 2021-10-07T06:38:03Z

I looked at the rewrite of ModuleSerializationProxy in Scala in more detail and it LGTM.

serializing a module (object O extends Serializable, which gets a synthetic def writeReplace() = new ModuleSerializationProxy(classOf[O$])) gives the exact same byte array on 2.13.6 and with this PR
serializing / deserializing the module back and forth between 2.13.6 and this PR works
classfile diff looks fine: https://gist.github.com/lrytz/c75bf89a3bfd7618ad575ea7c8e1c183

On runtime systems where `java.lang.ClassValue` is not available `ClassValueCompat` re-computes the value on each `get` invocation. Co-authored-by: nwk37011 <nwk37011@gmail.com>

lrytz

Rebased and squashed.

Thank you, @nwk37011!

sideeffffect · 2021-10-12T08:49:38Z

src/library/scala/runtime/ClassValueCompat.scala

+    def remove(cls: Class[_]): Unit
+  }
+
+  private val classValueAvailable: Boolean = try {


Would it in any way be beneficial to make it lazy val? 🤔

I don't think so, the field is only initialized when the enclosing object is first accessed.

a tiny sequel to scala#9752

SethTisue · 2021-10-12T20:52:25Z

on JDK 17 we're seeing (at https://app.travis-ci.com/github/scala/scala/jobs/542786102):

[error] /home/travis/build/scala/scala/src/library/scala/runtime/ModuleSerializationProxy.scala:23:23: class AccessController in package security is deprecated
[error]         java.security.AccessController.doPrivileged((() => cls.getField("MODULE$").get(null)): PrivilegedExceptionAction[Object])
[error]                       ^

PR with fix: #9785

fiatjaf · 2022-07-26T12:42:08Z

I have a question related to this PR, so I thought I would just beg for attention here. Let me know if this is inappropriate: https://stackoverflow.com/questions/73042201/why-a-scala-2-13-app-is-trying-to-use-javaclassvalue-on-android-if-it-is-not-a

scala-jenkins added this to the 2.13.7 milestone Sep 7, 2021

nwk37011 force-pushed the classvaluecompat branch from 16fa232 to e18202b Compare September 7, 2021 04:16

SethTisue added the release-notes worth highlighting in next release notes label Sep 7, 2021

SethTisue requested a review from retronym September 7, 2021 14:54

lrytz reviewed Sep 8, 2021

View reviewed changes

nwk37011 force-pushed the classvaluecompat branch 3 times, most recently from c5a9d09 to 272907f Compare September 17, 2021 07:48

bishabosha reviewed Sep 17, 2021

View reviewed changes

src/library/scala/compat/ClassValueCompat.scala Outdated Show resolved Hide resolved

SethTisue marked this pull request as draft September 22, 2021 16:19

dwijnand reviewed Sep 22, 2021

View reviewed changes

src/library/scala/runtime/ModuleSerializationProxy.java Outdated Show resolved Hide resolved

lrytz reviewed Sep 30, 2021

View reviewed changes

sideeffffect reviewed Oct 2, 2021

View reviewed changes

src/library/scala/runtime/ClassValueCompat.scala Outdated Show resolved Hide resolved

sideeffffect reviewed Oct 5, 2021

View reviewed changes

src/library/scala/runtime/ClassValueCompat.scala Outdated Show resolved Hide resolved

nwk37011 force-pushed the classvaluecompat branch from c312f37 to b55334e Compare October 12, 2021 04:38

ClassValueCompat to support systems without java.lang.ClassValue

6c9dd4d

On runtime systems where `java.lang.ClassValue` is not available `ClassValueCompat` re-computes the value on each `get` invocation. Co-authored-by: nwk37011 <nwk37011@gmail.com>

lrytz force-pushed the classvaluecompat branch from b55334e to 6c9dd4d Compare October 12, 2021 08:32

lrytz marked this pull request as ready for review October 12, 2021 08:32

retronym approved these changes Oct 12, 2021

View reviewed changes

lrytz approved these changes Oct 12, 2021

View reviewed changes

lrytz enabled auto-merge October 12, 2021 08:38

sideeffffect reviewed Oct 12, 2021

View reviewed changes

lrytz merged commit 12ff1e3 into scala:2.13.x Oct 12, 2021

SethTisue added a commit to SethTisue/scala that referenced this pull request Oct 12, 2021

suppress JDK 17 deprecation warning in ModuleSerializationProxy

db9b07c

a tiny sequel to scala#9752

SethTisue mentioned this pull request Oct 12, 2021

suppress JDK 17 deprecation warning in ModuleSerializationProxy #9785

Merged

SethTisue changed the title ~~ClassValueCompat to support systems without java.lang.ClassValue~~ Add ClassValueCompat to support systems without java.lang.ClassValue Oct 29, 2021

SethTisue changed the title ~~Add ClassValueCompat to support systems without java.lang.ClassValue~~ Add ClassValueCompat to support systems without java.lang.ClassValue (such as Android) Oct 29, 2021

sideeffffect referenced this pull request in chenakam/scalroid Feb 21, 2023

update README

652a991

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ClassValueCompat` to support systems without `java.lang.ClassValue` (such as Android) #9752

Add `ClassValueCompat` to support systems without `java.lang.ClassValue` (such as Android) #9752

nwk37011 commented Sep 7, 2021

smarter commented Sep 7, 2021

lrytz left a comment

lrytz left a comment

lrytz Sep 30, 2021

SethTisue Sep 30, 2021 •

edited

nwk37011 Oct 1, 2021 •

edited

lrytz commented Oct 7, 2021

lrytz left a comment

sideeffffect Oct 12, 2021

lrytz Oct 12, 2021

SethTisue commented Oct 12, 2021

fiatjaf commented Jul 26, 2022


		import scala.util.Try

		private[scala] abstract class ClassValueCompat[T] {

Add ClassValueCompat to support systems without java.lang.ClassValue (such as Android) #9752

Add ClassValueCompat to support systems without java.lang.ClassValue (such as Android) #9752

Conversation

nwk37011 commented Sep 7, 2021

smarter commented Sep 7, 2021

lrytz left a comment

Choose a reason for hiding this comment

lrytz left a comment

Choose a reason for hiding this comment

lrytz Sep 30, 2021

Choose a reason for hiding this comment

SethTisue Sep 30, 2021 • edited

Choose a reason for hiding this comment

nwk37011 Oct 1, 2021 • edited

Choose a reason for hiding this comment

lrytz commented Oct 7, 2021

lrytz left a comment

Choose a reason for hiding this comment

sideeffffect Oct 12, 2021

Choose a reason for hiding this comment

lrytz Oct 12, 2021

Choose a reason for hiding this comment

SethTisue commented Oct 12, 2021

fiatjaf commented Jul 26, 2022

Add `ClassValueCompat` to support systems without `java.lang.ClassValue` (such as Android) #9752

Add `ClassValueCompat` to support systems without `java.lang.ClassValue` (such as Android) #9752

SethTisue Sep 30, 2021 •

edited

nwk37011 Oct 1, 2021 •

edited