Share instance of ClassTag in a ClassValue based cache #7879

retronym · 2019-03-15T05:34:48Z

A dynamic alternative to #7862

lrytz · 2019-03-15T07:31:10Z

Also a simpler alternative :-) Looks good.

retronym · 2019-03-15T08:39:28Z

I'll take a look at the compiler performance (and profiles thereof) of the second commit to see if the cost of ClassValue hashmap lookup is perceptible or not.

retronym · 2019-03-15T09:30:39Z

The profile recorded one sample (of 5966) with ClassValue.get on the stack. The recorded times were a tiny bit slower than the previous commit, but there is some fluctuation due to JIT variance in those numbers.

We could create a way to enable the old behaviour with a system property to be more conservative.

I should also do a direct microbenchmark to see how the worst case looks.

retronym · 2019-05-30T06:27:24Z

The new benchmark actually improves (slightly) with the ClassValue cache.

[info] Benchmark                         Mode  Cnt   Score   Error  Units
[info] ClassTagBenchmark.lookupClassTag  avgt   20  53.383 ± 0.638  ns/op // Baseline
[info] ClassTagBenchmark.lookupClassTag  avgt   20  37.261 ± 0.308  ns/op // Optimize ClassTag.apply
...
[info] ClassTagBenchmark.lookupClassTag  avgt   20  35.012 ± 0.216  ns/op // Share instance of ClassTag ...

retronym · 2019-05-30T06:30:00Z

/synch

mkeskells · 2019-05-30T06:55:23Z

src/library/scala/reflect/ClassTag.scala

-      case _                        => new GenericClassTag[T](runtimeClass1)
-    }
+  def apply[T](runtimeClass1: jClass[_]): ClassTag[T] = runtimeClass1 match {
+    case x if x.isPrimitive =>  primitiveClassTag(runtimeClass1)


is it better to have this logic in the computeValue
depends on the frequency of lookup etc

Ideally we could add the special values to the cache on its construction, but I can see a hook to allow that in ClassValue

That sounds more regular, I've added a commit.

It slightly improves the lookup speed of refererence classtags.

[info] ClassTagBenchmark.lookupClassTag avgt 20 33.653 ± 0.644 ns/op

retronym · 2019-05-31T01:10:14Z

I'm also applying this technique to TypeTag-s: #8112

lrytz · 2019-07-16T14:40:48Z

That seems ready. @retronym, squash and merge?

lrytz · 2019-07-16T14:42:35Z

And: it was originally against 2.12.x, then you moved it to 2.13, do you remember why?

lrytz · 2019-07-16T14:49:55Z

Hmm, could this prevent classes from being garbage collected?

lrytz · 2019-07-16T14:58:30Z

Ah, ClassValue uses weak references.

- Optimize ClassTag.apply to avoid testing for primitive classes one-by-one. - Share instance of ClassTag in a ClassValue based cache and rely on this in the compiler where we had previously hoisted hot instances.

retronym · 2019-07-16T21:49:48Z

Squashed and retargeted to 2.12.x

retronym · 2019-07-17T02:35:02Z

/synch

retronym · 2019-07-17T02:37:08Z

"validate-main — No corresponding job found on Jenkins. Failed to launch? Try /rebuild " looks like a bug in scabot when retargetting PRs from 2.13.x to 2.13.x.

mkurz · 2021-02-01T16:07:18Z

@retronym @lrytz This never made it into 2.13.x even though in #8242 it says PICK.
Will you forward port this finally or isn't it worth it? If you do, you also should forward port #9276 and #9361 afterwards to fix a classloader leak this patch here introduced.

retronym · 2021-02-02T10:49:06Z

Thanks for the reminder. I'll forward port it this week with the subsequent patch to avoid the leak.

See scala#7879 and scala#9276

retronym · 2021-02-05T05:39:27Z

Forward port proposed in #9487

See scala#7879 and scala#9276

scala-jenkins added this to the 2.12.9 milestone Mar 15, 2019

retronym mentioned this pull request Mar 15, 2019

Use invokedynamic to statically cache materialized class tags (initially disabled by default) #7862

Closed

retronym requested a review from lrytz March 15, 2019 05:37

retronym force-pushed the classtag-cache branch from 3e621d2 to b5d7db6 Compare March 15, 2019 05:38

retronym added the WIP label Mar 20, 2019

retronym changed the base branch from 2.12.x to 2.13.x May 30, 2019 06:25

retronym force-pushed the classtag-cache branch from fafd54a to 1455b51 Compare May 30, 2019 06:25

retronym marked this pull request as ready for review May 30, 2019 06:25

retronym removed the WIP label May 30, 2019

mkeskells reviewed May 30, 2019

View reviewed changes

retronym force-pushed the classtag-cache branch from cc3c270 to b8847c5 Compare May 30, 2019 07:12

lrytz modified the milestones: 2.12.9, 2.13.1 Jul 16, 2019

Optimise ClassTag lookup

9f7866c

- Optimize ClassTag.apply to avoid testing for primitive classes one-by-one. - Share instance of ClassTag in a ClassValue based cache and rely on this in the compiler where we had previously hoisted hot instances.

retronym force-pushed the classtag-cache branch from b8847c5 to 9f7866c Compare July 16, 2019 21:49

retronym changed the base branch from 2.13.x to 2.12.x July 16, 2019 21:49

retronym merged commit b5029ab into scala:2.12.x Jul 17, 2019

retronym modified the milestones: 2.13.1, 2.12.9 Jul 17, 2019

SethTisue added the performance the need for speed. usually compiler performance, sometimes runtime performance. label Jul 18, 2019

lrytz mentioned this pull request Oct 26, 2020

fix ClassTag#cache leak #9276

Merged

retronym added a commit to retronym/scala that referenced this pull request Feb 5, 2021

Forward port ClassTag caching from 2.12.x

7bdc72d

See scala#7879 and scala#9276

retronym mentioned this pull request Feb 5, 2021

Forward port ClassTag caching from 2.12.x #9487

Merged

retronym added a commit to retronym/scala that referenced this pull request Feb 7, 2021

Forward port ClassTag caching from 2.12.x

3f2203b

See scala#7879 and scala#9276

lrytz pushed a commit to retronym/scala that referenced this pull request Feb 8, 2021

Forward port ClassTag caching from 2.12.x

998477d

See scala#7879 and scala#9276

nwk37011 mentioned this pull request Sep 7, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share instance of ClassTag in a ClassValue based cache #7879

Share instance of ClassTag in a ClassValue based cache #7879

retronym commented Mar 15, 2019

lrytz commented Mar 15, 2019

retronym commented Mar 15, 2019 •

edited

retronym commented Mar 15, 2019

retronym commented May 30, 2019 •

edited

retronym commented May 30, 2019

mkeskells May 30, 2019

retronym May 30, 2019

retronym commented May 31, 2019

lrytz commented Jul 16, 2019

lrytz commented Jul 16, 2019

lrytz commented Jul 16, 2019

lrytz commented Jul 16, 2019

retronym commented Jul 16, 2019 •

edited

retronym commented Jul 17, 2019

retronym commented Jul 17, 2019

mkurz commented Feb 1, 2021

retronym commented Feb 2, 2021

retronym commented Feb 5, 2021

Share instance of ClassTag in a ClassValue based cache #7879

Share instance of ClassTag in a ClassValue based cache #7879

Conversation

retronym commented Mar 15, 2019

lrytz commented Mar 15, 2019

retronym commented Mar 15, 2019 • edited

retronym commented Mar 15, 2019

retronym commented May 30, 2019 • edited

retronym commented May 30, 2019

mkeskells May 30, 2019

Choose a reason for hiding this comment

retronym May 30, 2019

Choose a reason for hiding this comment

retronym commented May 31, 2019

lrytz commented Jul 16, 2019

lrytz commented Jul 16, 2019

lrytz commented Jul 16, 2019

lrytz commented Jul 16, 2019

retronym commented Jul 16, 2019 • edited

retronym commented Jul 17, 2019

retronym commented Jul 17, 2019

mkurz commented Feb 1, 2021

retronym commented Feb 2, 2021

retronym commented Feb 5, 2021

retronym commented Mar 15, 2019 •

edited

retronym commented May 30, 2019 •

edited

retronym commented Jul 16, 2019 •

edited