Skip to content

Only look up the system prop controlling class tag caching once#9361

Merged
SethTisue merged 1 commit intoscala:2.12.xfrom
retronym:faster/class-tag-cache-property-check
Dec 4, 2020
Merged

Only look up the system prop controlling class tag caching once#9361
SethTisue merged 1 commit intoscala:2.12.xfrom
retronym:faster/class-tag-cache-property-check

Conversation

@retronym
Copy link
Member

@retronym retronym commented Dec 4, 2020

Fixes a performance regression in the opt-out mechanism I added to #9276 but neglected to benchmark.

Prior to this PR (with the perf regression)

[info] ClassTagBenchmark.lookupClassTag  avgt   20  257.807 ± 12.567  ns/op

This PR:

[info] Benchmark                         Mode  Cnt   Score   Error  Units
[info] ClassTagBenchmark.lookupClassTag  avgt   20  43.603 ± 0.728  ns/op

With the if (cacheDiabled) removed:

[info] Benchmark                         Mode  Cnt    Score    Error  Units
[info] ClassTagBenchmark.lookupClassTag  avgt   20  42.292 ± 0.789  ns/op

The extra if (cacheDisabled) now seems to have no significant performance cost, as we need.

@scala-jenkins scala-jenkins added this to the 2.12.14 milestone Dec 4, 2020
@retronym retronym modified the milestones: 2.12.14, 2.12.13 Dec 4, 2020
@retronym retronym requested a review from SethTisue December 4, 2020 03:59
@SethTisue SethTisue merged commit 5c9699d into scala:2.12.x Dec 4, 2020
@mkurz mkurz mentioned this pull request Dec 10, 2020
@mkeskells
Copy link
Contributor

mkeskells commented Dec 10, 2020

this also removed an allocation caused by cacheDisabledProp.isSet

@som-snytt
Copy link
Contributor

No one wondered why those sys API aren't deprecated or similarly discouraged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants