Performance optimizations for Zinc #492

retronym · 2018-02-17T04:30:44Z

No description provided.

typesafe-tools · 2018-02-17T05:48:53Z

The validator has checked the following projects against Scala 2.12,
tested using dbuild, projects built on top of each other.

Project	Reference	Commit
sbt	1.x	sbt/sbt@`78f7658`
zinc	pull/492/head	`3d1dd67`
io	1.x	sbt/io@`7f8c185`
librarymanagement	1.x	sbt/librarymanagement@`0d21ae6`
util	1.x	sbt/util@`9891f07`
website	1.x

✅ The result is: SUCCESS
(restart)

jvican · 2018-02-21T10:30:41Z

Changes look good to me. Had a look at the CI, bincompat checks are failing. We should probably whitelist them.

[error]  * method hashTypeParameters(scala.collection.Seq)Unit in class xsbt.api.HashAPI's type is different in current version, where it is (Array[xsbti.api.TypeParameter])Unit instead of (scala.collection.Seq)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.HashAPI.hashTypeParameters")
[error]  * method hashAnnotations(scala.collection.Seq)Unit in class xsbt.api.HashAPI's type is different in current version, where it is (Array[xsbti.api.Annotation])Unit instead of (scala.collection.Seq)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.HashAPI.hashAnnotations")
[error]  * method hashParameters(scala.collection.Seq,xsbti.api.Type)Unit in class xsbt.api.HashAPI's type is different in current version, where it is (Array[xsbti.api.TypeParameter],xsbti.api.Type)Unit instead of (scala.collection.Seq,xsbti.api.Type)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.HashAPI.hashParameters")
[error]  * method hashDefinitionsWithExtraHashes(scala.collection.Seq)Unit in class xsbt.api.HashAPI does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("xsbt.api.HashAPI.hashDefinitionsWithExtraHashes")
[error]  * method hashSeq(scala.collection.Seq,scala.Function1)Unit in class xsbt.api.HashAPI does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("xsbt.api.HashAPI.hashSeq")
[error]  * method hashValueParameters(scala.collection.Seq)Unit in class xsbt.api.HashAPI's type is different in current version, where it is (Array[xsbti.api.ParameterList])Unit instead of (scala.collection.Seq)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.HashAPI.hashValueParameters")
[error]  * method hashAnnotationArguments(scala.collection.Seq)Unit in class xsbt.api.HashAPI's type is different in current version, where it is (Array[xsbti.api.AnnotationArgument])Unit instead of (scala.collection.Seq)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.HashAPI.hashAnnotationArguments")
[error]  * method hashTypes(scala.collection.Seq,Boolean)Unit in class xsbt.api.HashAPI's type is different in current version, where it is (Array[xsbti.api.Type],Boolean)Unit instead of (scala.collection.Seq,Boolean)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.HashAPI.hashTypes")
[error]  * method visitTypeParameters(scala.collection.Seq)Unit in class xsbt.api.Visit's type is different in current version, where it is (Array[xsbti.api.TypeParameter])Unit instead of (scala.collection.Seq)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitTypeParameters")
[error]  * method visitDefinitions(scala.collection.Seq)Unit in class xsbt.api.Visit's type is different in current version, where it is (Array[xsbti.api.Definition])Unit instead of (scala.collection.Seq)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitDefinitions")
[error]  * method visitAnnotationArguments(scala.collection.Seq)Unit in class xsbt.api.Visit's type is different in current version, where it is (Array[xsbti.api.AnnotationArgument])Unit instead of (scala.collection.Seq)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitAnnotationArguments")
[error]  * method visitAnnotations(scala.collection.Seq)Unit in class xsbt.api.Visit's type is different in current version, where it is (Array[xsbti.api.Annotation])Unit instead of (scala.collection.Seq)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitAnnotations")
[error]  * method visitValueParameters(scala.collection.Seq)Unit in class xsbt.api.Visit's type is different in current version, where it is (Array[xsbti.api.ParameterList])Unit instead of (scala.collection.Seq)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitValueParameters")
[error]  * method visitParameters(scala.collection.Seq,xsbti.api.Type)Unit in class xsbt.api.Visit's type is different in current version, where it is (Array[xsbti.api.TypeParameter],xsbti.api.Type)Unit instead of (scala.collection.Seq,xsbti.api.Type)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitParameters")
[error]  * method visitTypes(scala.collection.Seq)Unit in class xsbt.api.Visit's type is different in current version, where it is (Array[xsbti.api.Type])Unit instead of (scala.collection.Seq)Unit
[error] filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitTypes")

retronym · 2018-02-21T11:06:41Z

Yeah, I figured that wasn't really external API.

The PR is in a bit of messy state, I need to take some more time to rebase. I'd also like to try out your benchmarking rig to see which (if any) commits have a positive impact on runtimes. I drove these changes by collecting profiles with flight recorder.

To be conservative, we limit the inliner to annotated methods from within the current sources. The motiviation is to improve the efficiency of inlinable utility methods in ExtractAPI and Visit that will be addressed in the subsequent commits.

- hand inline enteringPhase to avoid closure allocation - avoid looking up getter/setters for non fields - optimize for common case of no annotations

This is not to improve performance, but rather to give cleaner profiles.

retronym · 2018-02-22T07:53:35Z

Here's the report for -prof gc for the scala-library corpus.

The last column is the allocation in B/op attributable to Zinc itself. This PR reduces that by about a third.

num  desc                      gc.alloc.rate.norm   <value>-#1       
1    baseline, zinc disabled   3509985410.667  
2    baseline  zinc enabled    5148114788.000       1638129378
3    patch, zinc enabled       4612042982.667       1102057572

I'm thinking of adding a mode to the benchmark rig that runs the compiler up to the ExtractAPI phase in @Setup, and loops over the ExtractAPI work in the benchmarked method. That will make it much easier to targetted profiling and tuning of Zinc. Of course, we must also validate the results in the broader context of the normal compilation run, as the benchmarks currently do.

retronym · 2018-02-22T07:55:16Z

internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala

-      val associated =
-        List(b, b.getterIn(b.enclClass), b.setterIn(b.enclClass)).filter(_ != NoSymbol)
-      associated.flatMap(ss => mkAnnotations(in, ss.annotations)).distinct.toArray
+      if (b.hasGetter) {


This change needs the most careful review. We should probably build a version that has the old and new implementations in place that asserts they agree.

Looks correct under the assumption that the compiler does not synthesize setters if there are no getters. We have a biggish community build in bloop (https://github.com/scalacenter/bloop/tree/master/build-integrations), I can try out the optimized Zinc version when this PR is ready and we assert that we don't break incremental compilation for existing projects. Unfortunately, these tests will need to be manual since there's no automated infrastructure for it aside from the scripted tests we have.

Is it possible to write out common cases as tests in https://github.com/sbt/zinc/blob/1.1.x/internal/compiler-bridge-test/src/test/scala/xsbt/ExtractAPISpecification.scala?

retronym · 2018-02-22T07:56:43Z

internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala

  // these caches are necessary for correctness
-  private[this] val structureCache = perRunCaches.newMap[Symbol, xsbti.api.Structure]()
+  private[this] val structureCache = new java.util.HashMap[Symbol, xsbti.api.Structure]()


The motivation for using Java maps was to avoid the cost of generalized equality and hashing. Usually I'd reach for an AnyRefMap, but AFAICT this code needs to cross compile to Scala 2.10, so it was easier to just use a Java Map.

retronym · 2018-02-22T08:56:36Z

I'm thinking of adding a mode to the benchmark rig that runs the compiler up to the ExtractAPI phase in @Setup, and loops over the ExtractAPI work in the benchmarked method.

It requires a few hacks, but seem to work: https://github.com/retronym/zinc/tree/topic/focus

WDYT?

[info] Benchmark                                                                       (_tempDir)  (zincEnabled)    Mode  Cnt           Score          Error   Units
[info] HotScalacMicroBenchmark.compile                                   /tmp/zinc-bench-baseline           true  sample   18        3888.819 ±      211.550   ms/op
[info] HotScalacMicroBenchmark.compile:compile·p0.00                     /tmp/zinc-bench-baseline           true  sample             3703.570                  ms/op
[info] HotScalacMicroBenchmark.compile:compile·p0.50                     /tmp/zinc-bench-baseline           true  sample             3839.885                  ms/op
[info] HotScalacMicroBenchmark.compile:compile·p0.90                     /tmp/zinc-bench-baseline           true  sample             4156.136                  ms/op
[info] HotScalacMicroBenchmark.compile:compile·p0.95                     /tmp/zinc-bench-baseline           true  sample             4680.843                  ms/op
[info] HotScalacMicroBenchmark.compile:compile·p0.99                     /tmp/zinc-bench-baseline           true  sample             4680.843                  ms/op
[info] HotScalacMicroBenchmark.compile:compile·p0.999                    /tmp/zinc-bench-baseline           true  sample             4680.843                  ms/op
[info] HotScalacMicroBenchmark.compile:compile·p0.9999                   /tmp/zinc-bench-baseline           true  sample             4680.843                  ms/op
[info] HotScalacMicroBenchmark.compile:compile·p1.00                     /tmp/zinc-bench-baseline           true  sample             4680.843                  ms/op
[info] HotScalacMicroBenchmark.compile:·gc.alloc.rate                    /tmp/zinc-bench-baseline           true  sample    6         281.356 ±       29.502  MB/sec
[info] HotScalacMicroBenchmark.compile:·gc.alloc.rate.norm               /tmp/zinc-bench-baseline           true  sample    6  1198942299.556 ±   819406.113    B/op
[info] HotScalacMicroBenchmark.compile:·gc.churn.PS_Eden_Space           /tmp/zinc-bench-baseline           true  sample    6         281.810 ±       29.267  MB/sec
[info] HotScalacMicroBenchmark.compile:·gc.churn.PS_Eden_Space.norm      /tmp/zinc-bench-baseline           true  sample    6  1200910416.889 ± 19759251.396    B/op
[info] HotScalacMicroBenchmark.compile:·gc.churn.PS_Old_Gen              /tmp/zinc-bench-baseline           true  sample    6          30.626 ±        4.740  MB/sec
[info] HotScalacMicroBenchmark.compile:·gc.churn.PS_Old_Gen.norm         /tmp/zinc-bench-baseline           true  sample    6   130446065.333 ±  9252509.827    B/op
[info] HotScalacMicroBenchmark.compile:·gc.churn.PS_Survivor_Space       /tmp/zinc-bench-baseline           true  sample    6          18.590 ±        6.947  MB/sec
[info] HotScalacMicroBenchmark.compile:·gc.churn.PS_Survivor_Space.norm  /tmp/zinc-bench-baseline           true  sample    6    79081034.667 ± 24937547.953    B/op
[info] HotScalacMicroBenchmark.compile:·gc.count                         /tmp/zinc-bench-baseline           true  sample    6         311.000                 counts
[info] HotScalacMicroBenchmark.compile:·gc.time                          /tmp/zinc-bench-baseline           true  sample    6       10263.000                     ms

jvican · 2018-02-22T10:24:09Z

internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala

+  // on non-fields.
+  private def annotations(in: Symbol, s: Symbol): Array[xsbti.api.Annotation] = {
+    val saved = phase
+    phase = currentRun.typerPhase


Is time-travelling with Scalac's API generally inefficient or impedes inlining? I see that you're inlining the body manually here.

Does the on-the-fly compilation of the compiler interface enable the
optimiser? If not, hand inlining here is helpful.

https://github.com/sbt/zinc/blob/v1.1.1/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/AnalyzingCompiler.scala#L350:

compiler(sourceFiles, classpath, outputDirectory, "-nowarn" :: Nil)

It seems like we just pass in "-nowarn".

It would be interesting to add the optimiser flags in here and benchmark the bridge, WDYT?

I think it makes sense, especially if we can observe some effect. Is there a safe flag that would work for all Scala versions, or do we have to optimize the list of optimize flag per Scala versions?

The compiler options and bugs are version dependent. I would be inclined to leave it off, at least by default. Exposing these options as config might be useful for experimentation.

Demo: ``` sbt:zinc Root> zincBenchmarks/jmh:run -p _tempDir=/tmp/zinc-bench-baseline HotScalacApiExtractBenchmark -wi 10 -i 6 -f1 -prof jmh.extras.JFR:flameGraphOpts=--minwidth,1;verbose=true [info] # Run complete. Total time: 00:04:24 [info] Benchmark (_tempDir) (zincEnabled) Mode Cnt Score Error Units [info] HotScalacApiExtractBenchmark.compile /tmp/zinc-bench-baseline true sample 16 4962.386 ± 119.982 ms/op [info] HotScalacApiExtractBenchmark.compile:JFR /tmp/zinc-bench-baseline true sample NaN N/A [info] HotScalacApiExtractBenchmark.compile:compile·p0.00 /tmp/zinc-bench-baseline true sample 4823.450 ms/op [info] HotScalacApiExtractBenchmark.compile:compile·p0.50 /tmp/zinc-bench-baseline true sample 4928.307 ms/op [info] HotScalacApiExtractBenchmark.compile:compile·p0.90 /tmp/zinc-bench-baseline true sample 5163.188 ms/op [info] HotScalacApiExtractBenchmark.compile:compile·p0.95 /tmp/zinc-bench-baseline true sample 5192.548 ms/op [info] HotScalacApiExtractBenchmark.compile:compile·p0.99 /tmp/zinc-bench-baseline true sample 5192.548 ms/op [info] HotScalacApiExtractBenchmark.compile:compile·p0.999 /tmp/zinc-bench-baseline true sample 5192.548 ms/op [info] HotScalacApiExtractBenchmark.compile:compile·p0.9999 /tmp/zinc-bench-baseline true sample 5192.548 ms/op [info] HotScalacApiExtractBenchmark.compile:compile·p1.00 /tmp/zinc-bench-baseline true sample 5192.548 ms/op ``` https://www.dropbox.com/sh/f06rz1ykteaalxz/AAB1myNOxMOZ9Rjsq6XBDSfHa?dl=0 Which after optimizations in sbt#492 Looks like: ``` [info] Benchmark (_tempDir) (zincEnabled) Mode Cnt Score Error Units [info] HotScalacMicroBenchmark.compile /tmp/zinc-bench-baseline true sample 18 4290.773 ± 206.870 ms/op [info] HotScalacMicroBenchmark.compile:JFR /tmp/zinc-bench-baseline true sample NaN N/A [info] HotScalacMicroBenchmark.compile:compile·p0.00 /tmp/zinc-bench-baseline true sample 3976.200 ms/op [info] HotScalacMicroBenchmark.compile:compile·p0.50 /tmp/zinc-bench-baseline true sample 4188.013 ms/op [info] HotScalacMicroBenchmark.compile:compile·p0.90 /tmp/zinc-bench-baseline true sample 4684.199 ms/op [info] HotScalacMicroBenchmark.compile:compile·p0.95 /tmp/zinc-bench-baseline true sample 4714.398 ms/op [info] HotScalacMicroBenchmark.compile:compile·p0.99 /tmp/zinc-bench-baseline true sample 4714.398 ms/op [info] HotScalacMicroBenchmark.compile:compile·p0.999 /tmp/zinc-bench-baseline true sample 4714.398 ms/op [info] HotScalacMicroBenchmark.compile:compile·p0.9999 /tmp/zinc-bench-baseline true sample 4714.398 ms/op [info] HotScalacMicroBenchmark.compile:compile·p1.00 /tmp/zinc-bench-baseline true sample 4714.398 ms/op ``` https://www.dropbox.com/sh/41o382xb19y331v/AADMmaFBajbEuG_yysprM6zAa?dl=0

jvican

LGTM, this is awesome 👍

dwijnand · 2018-03-05T11:10:40Z

build.sbt

+         exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitAnnotations"),
+         exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitValueParameters"),
+         exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitParameters"),
+         exclude[IncompatibleMethTypeProblem]("xsbt.api.Visit.visitTypes"),


the "api" in "xsbt.api" makes me concerned: is this public API?

I think the "api" here refers the the functionality of this code (visiting the API).

It would be possible to leave deprecated methods in place with the old signatures if needed.

jvican · 2018-04-13T10:29:32Z

@dwijnand Is there something here you'd like to see address before we merge?

dwijnand

492 you're clear for landing

eed3si9n · 2018-05-05T07:03:59Z

build.sbt

  scalacOptions := {
    val old = scalacOptions.value
    scalaBinaryVersion.value match {
-      case "2.12" => old
+      case "2.12" => old ++ List("-opt-inline-from:<sources>", "-opt:l:inline", "-Yopt-inline-heuristics:at-inline-annotated")


Due to #537, does this mean that incremental compilation of Zinc itself no longer works?

retronym changed the base branch from 1.x to 1.1.x February 17, 2018 04:35

retronym force-pushed the faster/hash-array-direct-use branch 3 times, most recently from d3f7c50 to 67207c3 Compare February 17, 2018 04:40

retronym force-pushed the faster/hash-array-direct-use branch 3 times, most recently from ca5af2f to 006cb12 Compare February 20, 2018 00:59

jvican added binary api performance benchmark labels Feb 21, 2018

Enable the Scala 2.12 inliner

554f4ae

To be conservative, we limit the inliner to annotated methods from within the current sources. The motiviation is to improve the efficiency of inlinable utility methods in ExtractAPI and Visit that will be addressed in the subsequent commits.

retronym force-pushed the faster/hash-array-direct-use branch 3 times, most recently from ce82bc4 to 7b3f846 Compare February 22, 2018 05:26

retronym added 7 commits February 22, 2018 15:37

Avoid use of WrappedArray in favour of direct Array usage

351ab3d

Avoid boxing in tuples in name hashing

0321c71

Optimize annotation extraction

be05038

- hand inline enteringPhase to avoid closure allocation - avoid looking up getter/setters for non fields - optimize for common case of no annotations

Use iterators rather than foreach

8ca9eff

This is not to improve performance, but rather to give cleaner profiles.

Hoist functions to reduce allocations in hot code path

937c0c0

Use AnyRefMap

3e4b3b4

Use Java collections

52959c9

retronym force-pushed the faster/hash-array-direct-use branch 2 times, most recently from 908486e to 52959c9 Compare February 22, 2018 06:31

retronym commented Feb 22, 2018

View reviewed changes

jvican reviewed Feb 22, 2018

View reviewed changes

retronym mentioned this pull request Feb 23, 2018

Benchmark the API extract phase in isolation #501

Closed

Add whitelist entries for HashAPI changes

9a94418

retronym changed the title ~~WIP Performance optimizations for Zinc~~ Performance optimizations for Zinc Mar 1, 2018

jvican approved these changes Mar 2, 2018

View reviewed changes

dwijnand reviewed Mar 5, 2018

View reviewed changes

dwijnand approved these changes Apr 23, 2018

View reviewed changes

dwijnand merged commit 4eb82ee into sbt:1.1.x Apr 23, 2018

dwijnand modified the milestones: 1.1.5, 1.1.6 Apr 23, 2018

eed3si9n reviewed May 5, 2018

View reviewed changes

eed3si9n mentioned this pull request May 6, 2018

compiler does not work with "-opt:l:method" on JDK10 scala/bug#10717

Closed

eed3si9n mentioned this pull request Feb 25, 2020

Overcompilation after whitespace change #718

Closed

retronym mentioned this pull request Jun 18, 2020

Ignore annotations are inadvertently added to NoSymbol #798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance optimizations for Zinc #492

Performance optimizations for Zinc #492

retronym commented Feb 17, 2018

typesafe-tools commented Feb 17, 2018

jvican commented Feb 21, 2018

retronym commented Feb 21, 2018 •

edited

Loading

retronym commented Feb 22, 2018

retronym Feb 22, 2018

jvican Feb 22, 2018 •

edited

Loading

eed3si9n Mar 1, 2018

retronym Feb 22, 2018

retronym commented Feb 22, 2018 •

edited

Loading

jvican Feb 22, 2018

retronym Mar 1, 2018

eed3si9n Mar 1, 2018

jvican Mar 1, 2018 •

edited

Loading

eed3si9n Mar 1, 2018

retronym Mar 1, 2018

jvican left a comment

dwijnand Mar 5, 2018

retronym Mar 6, 2018

jvican commented Apr 13, 2018

dwijnand left a comment

eed3si9n May 5, 2018

Performance optimizations for Zinc #492

Performance optimizations for Zinc #492

Conversation

retronym commented Feb 17, 2018

typesafe-tools commented Feb 17, 2018

jvican commented Feb 21, 2018

retronym commented Feb 21, 2018 • edited Loading

retronym commented Feb 22, 2018

Choose a reason for hiding this comment

jvican Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retronym commented Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Apr 13, 2018

dwijnand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retronym commented Feb 21, 2018 •

edited

Loading

jvican Feb 22, 2018 •

edited

Loading

retronym commented Feb 22, 2018 •

edited

Loading

jvican Mar 1, 2018 •

edited

Loading