Skip to content
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

Outline Typing #44

Closed
wants to merge 27 commits into from
Closed

Outline Typing #44

wants to merge 27 commits into from

Conversation

retronym
Copy link
Owner

@retronym retronym commented Apr 30, 2019

A new mode for PipelineMain that can shorten the critical path of a build and extract more parallelism from the post-typer compiler phases (at the cost of instantiating multiple instances of Global).

First, we compile all sources with -Youtline -Ystop-after:pickler. The first option disables typechecking of the RHS of definitions that have ascribed types.

The RHS is not skipped when it contains a super call that might need a super accessor. (hat tip to @smarter for finding this corner case in dotty). Are there more like it that we haven't identified?

The pickles are then exported and made available to downstream compilation, as is also the case with the strategy=Pipeline. The sources for the current projects are then compiled in parallel in chunks, also using the exported pickles on the classpath.

Optionally, we can share the name table between Global instances to reduce the memory and bookkeeping overhead of the traditional approach of having this inside the cake. For backwards binary compatibility reasons, this is implemented with an abundance of casts and other hacks.

Testing this out guardian-frontend. In the past I've experimented with the full multi-module build, but in this case I want to focus on the common project that all other depend on and that is the bottleneck for a full clean build.

Pipeline + Outline

$ mkdir -p /tmp/pickle-cache && ./build/pack/bin/scala -J-Dsun.reflect.inflationThreshold=0  -J-Xmx4G -Dscala.pipeline.cache.name.table=true -Dscalac.filebasedcache.defer.close.ms=30000 -Dscala.pipeline.picklecache=/tmp/pickle-cache -Dscala.pipeline.group.size=128  -Dscala.pipeline.strategy=outlinetypepipeline -Dscala.pipeline.strip.external.classpath=true -Dscala.pipeline.cache.macro.classloader=true -Dscala.pipeline.use.jar=true scala.tools.nsc.PipelineMainTest /Users/jz/code/guardian-frontend/common/target/compile.args
...
====== ITERATION 20=======
parallelism = 8, strategy = OutlineTypePipeline
Exported external classpath in 4 ms
Wrote project dependency graph to: /Users/jz/code/scala/projects.dot
/Users/jz/code/guardian-frontend/common:compile: scalac outline: start
/Users/jz/code/guardian-frontend/common:compile: scala outline: done 1378 ms
/Users/jz/code/guardian-frontend/common:compile: scalac (1/4): start
/Users/jz/code/guardian-frontend/common:compile: scalac (2/4): start
/Users/jz/code/guardian-frontend/common:compile: scalac (4/4): start
/Users/jz/code/guardian-frontend/common:compile: scalac (3/4): start
/Users/jz/code/guardian-frontend/common:compile: scalac (4/4): done 1729 ms
/Users/jz/code/guardian-frontend/common:compile: scalac (3/4): done 1898 ms
/Users/jz/code/guardian-frontend/common:compile: scalac (1/4): done 3074 ms
/Users/jz/code/guardian-frontend/common:compile: scalac (2/4): done 4698 ms
PROGRESS: 6 / 6
 Wall Clock: 6077 ms
Chrome trace written to /Users/jz/code/scala/build-OutlineTypePipeline-20.trace

Pipeline

$ ... -Dscala.pipeline.strategy=pipeline ...
====== ITERATION 20=======
parallelism = 8, strategy = Pipeline
Exported external classpath in 4 ms
Wrote project dependency graph to: /Users/jz/code/scala/projects.dot
/Users/jz/code/guardian-frontend/common:compile: scalac: start
/Users/jz/code/guardian-frontend/common:compile: scalac outline: done 4232 ms
/Users/jz/code/guardian-frontend/common:compile: scalac: exported pickles 52 ms
/Users/jz/code/guardian-frontend/common:compile: scalac: done 4523 ms
PROGRESS: 3 / 3
 Wall Clock: 8807 ms
Chrome trace written to /Users/jz/code/scala/build-Pipeline-20.trace

Traditional

$ ... -Dscala.pipeline.strategy=traditional ...
====== ITERATION 20=======
parallelism = 8, strategy = Traditional
Wrote project dependency graph to: /Users/jz/code/scala/projects.dot
/Users/jz/code/guardian-frontend/common:compile: scalac (1/1): start
/Users/jz/code/guardian-frontend/common:compile: scalac (1/1): done 9343 ms
PROGRESS: 3 / 3
Wall Clock: 9409 ms
Chrome trace written to /Users/jz/code/scala/build-Traditional-20.trace

Summary

  • The main benefit of the new strategy here is reducing the "time-to-pickles" down to 1.4s (vs 9.3 for traditional vs 4.2 for non-outline pipeline)
  • The wall clock time is also reduced somewhat (6.1s vs 8.8s), which may be interesting in itself when enough cores are available. There will be rapidly diminishing returns as the chunk size gets too small, however, due to the current approach of using a Global instance per-chunk.

Caveats

  • Incompatible with the non-local optimizations in the backend as .class files for symbols backed by exported pickles aren't available. This shouldn't be a big problem in "dev mode".
  • To get the most of out of the caching of macro classloaders, a hand crafted -Ymacro-classpath setting is needed with the macro implementation.

retronym and others added 3 commits April 10, 2019 10:08
The old approach of using the first, last, and middle characters
only lays a trap for generate names that have little or no entropy
at these locations.

For instance, fresh existential names generated in "as seen
from" operations are one such case, and when compiling large
batches of files the name table can become imbalanced.

This seems to be the bottleneck compiling the enourmous
(generated) test suite for ScalaTest itself:

   scala/scala-dev#246 (comment)

This commit uses all characters to compute the hashCode.

It improves the compilation time of ScalaTest tests from
487s to 349s (0.71x).

It would still be useful to avoid generating these fresh names
with a global counter, as this represents a steady name leak
in long-lived Globals (e.g. the presentation compiler.)
Fallback to the old style when the more than 64 varargs are provided.

Backport of a limit introduced in 2.13.x in scala#7678
@retronym
Copy link
Owner Author

/cc @smarter @jvican

@smarter
Copy link

smarter commented Apr 30, 2019

The RHS is not skipped when it contains a super call that might need a super accessor. (hat tip to @smarter for finding this corner case in dotty). Are there more like it that we haven't identified?

I think protected accessors are generate pre-pickler in Scala 2 (in Dotty they aren't, but I'm not 100% sure that handles all cases correctly), so that probably leads to the same issue we have with super accessors.

Regarding performance, in scala/scala3#4767 I observed that the lock in https://github.com/lampepfl/dotty/blob/29aeb02be0c08a20373027a0d80deaa87cb699ff/compiler/src/dotty/tools/dotc/classpath/ZipAndJarFileLookupFactory.scala#L181 (which corresponds to https://github.com/scala/scala/blob/bec2441a24cbf3159a1118049b857a5ae7c452eb/src/compiler/scala/tools/nsc/classpath/ZipAndJarFileLookupFactory.scala#L253 in scala/scala) seemed to be a bottleneck, but I haven't had time to experiment with alternative approaches like having an option to make the cache per compiler instance.

@retronym
Copy link
Owner Author

retronym commented May 1, 2019

@smarter AFAICT protected accessors are only called from elsewhere in the same compilation unit which wouldn't be a problem. We should follow dotty's lead in Scala 2.14. and move generation post-pickler IMO.

I've never really looked at this area before. Some aspects look wrong.

We don't emit scala protected methods as ACC_PROTECTED, apparently because "protected cannot be used, since inner classes 'see' protected members,". But can't we just use protected accessors for these? (Often these aren't actually needed if the callsite and definition are in the same package)

So that leaves test/files/jvm/protectedacc.scala with examples of times we actually do emit a protected accessor. The only one generated in that test is for:

      class JavaInteraction extends java.io.CharArrayReader {
        <artifact> def protected$count(x$1: p.b.JavaInteraction): Int = x$1.count;
        <artifact> def protected$setcount(x$1: p.b.JavaInteraction, x$2: Int): Unit = x$1.count = x$2;
        <paramaccessor> private[this] val arr: Array[Char] = _;
        def <init>(arr: Array[Char]): p.b.JavaInteraction = {
          JavaInteraction.super.<init>(arr);
          ()
        };
        class Inner extends scala.AnyRef {
          def <init>(): JavaInteraction.this.Inner = {
            Inner.super.<init>();
            ()
          };
          def m: Unit = {
            scala.Console.println("count before: ".+(JavaInteraction.this.protected$count(JavaInteraction.this)));
            JavaInteraction.this.protected$setcount(JavaInteraction.this, JavaInteraction.this.protected$count(JavaInteraction.this).+(1));
            scala.Console.println("count after: ".+(JavaInteraction.this.protected$count(JavaInteraction.this)))
          }
        }
      };

It seems to me like protected$count should be emitted as (Scala) private, in order to give it an expanded name later on.

For bonus points we would emit it as a static method like Java's access$N methods.

mpollmeier and others added 11 commits May 2, 2019 12:48
to trigger XSS vuln, simply paste this into the search bar:
```
"\><img/src='1'onerror=alert(777111)>{{7*7}}
```

all credit for finding the vulnerability goes to *Yeasir Arafat* <skylinearafat@gmail.com>
Call TypeName.toTermName less frequently.
rather than escaping the search string, which breaks the search for
e.g. `:+`

solution contributed by NthPortal in scala#8018 (comment)
fix XSS vulnerability in scaladoc search by escaping the input parameter
correct jansi version in intellij setup
Limit string interpolation intrinsic to avoid compiler SOE
Cancel in-progress timer task on a cache hit. This avoids
reducing the effective deferred close delay when the old
timer task fires and sees a ref count of zero, even though
the ref count has since been positive.
@retronym retronym force-pushed the topic/outline-typing branch from 1781a17 to ae097be Compare May 8, 2019 07:29
retronym and others added 7 commits May 9, 2019 08:15
Due to alignment, TermName_R (which doesn't cache the provided string
for toString) takes up just as much space as TermName_S.

The code ends up somewhat easier to read with by just encoding the
difference with the a nullable field.
…leanup

Cleanups around caching of classpath/classloaders
```
scala> :power
Power mode enabled. :phase is at typer.
import scala.tools.nsc._, intp.global._, definitions._
Try :help or completions for vals._ and power._

scala> val t = TermName("abcdefghijklmnopqrstuvwxyz")
t: $r.intp.global.TermName = abcdefghijklmnopqrstuvwxyz

scala> t.subName(0, 25)
res0: $r.intp.global.TermName = abcdefghijklmnopqrstuvwxy

scala> res0.start
res1: Int = 474232

scala> t.start
res2: Int = 474232
```
@retronym retronym force-pushed the topic/outline-typing branch from 287c3b2 to 9e3e44d Compare May 14, 2019 04:31
@retronym retronym force-pushed the topic/outline-typing branch from 9e3e44d to c74a9b1 Compare May 20, 2019 03:37
Demo:

```
$ cat sandbox/test.scala && (scalac-ref 2.12.x -Ytyper-debug sandbox/test.scala 2>&1) > /tmp/old.log && (qscalac -Ytyper-debug sandbox/test.scala 2>&1) > /tmp/new.log && diff -U1000 /tmp/{old,new}.log
```

```scala
trait C {
  type X
  def foo: X
}
```

```diff
--- /tmp/old.log    2019-05-20 13:56:45.000000000 +1000
+++ /tmp/new.log    2019-05-20 13:56:47.000000000 +1000
@@ -1,12 +1,10 @@
 |-- <empty> EXPRmode-POLYmode-QUALmode (site: package <root>)
+|-- <empty> EXPRmode-POLYmode-QUALmode (site: package <root>)
 |    \-> <empty>.type
 |-- class C BYVALmode-EXPRmode (site: package <empty>)
 |    |-- X BYVALmode-EXPRmode (site: trait C)
 |    |    \-> [type X] C.this.X
 |    |-- def foo BYVALmode-EXPRmode (site: trait C)
 |    |    |-- X TYPEmode (site: method foo in C)
 |    |    |    \-> C.this.X
-|    |    |-- X TYPEmode (site: method foo in C)
-|    |    |    \-> C.this.X
 |    |    \-> [def foo] => C.this.X
 |    \-> [trait C] C
```
Avoid typechecking val and def tpt-s twice
@retronym retronym force-pushed the topic/outline-typing branch 3 times, most recently from 1c1aabf to 4c41101 Compare May 22, 2019 03:49
  - Allow user specified reporter
    - funnel javac errors through it
    - funnel PipelineMain's logging through it, too.
  - Use a separate FileManager for each javac invocation to avoid an
    apparent race condition.
  - Expose config knobs programatically rather than only through
    system properties.
@retronym retronym force-pushed the topic/outline-typing branch from 4c41101 to c9d68d8 Compare May 22, 2019 04:15
In this new mode, the RHS of definitions is only typechecked
if the definition lacks an explicit type ascription, or or it
may contain a super call that is compiled to a trait super
accessor. Refer to the new test case for a motivating example.
@retronym retronym force-pushed the topic/outline-typing branch from c9d68d8 to 3cfb31c Compare May 22, 2019 04:28
@retronym retronym closed this May 22, 2019
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