The commit 388ed641fb added an autoplugin
that was compiled for every scripted test. Compiling autoplugins
introduces a fair bit of overhead because it can easily take 3-4 seconds
to compile with a cold compiler and even a warn compiler takes a second
or so. Removing the instrumentation caused 3 tests to fail:
1. genereated-root-no-publish relied on setUpScripted modifing the project
name. Explicitly setting the name in the project build.sbt fixed it.
2. cp-order I'm not sure why this change broke that test, but changing
the coursier classpath ordering setting does not automatically
trigger a reload on the next update. I have a more involved change
that makes changing coursier settings invalidate the update cache
but I'm markign the test as pending for now. It could be fixed by
adding a call to `update` after
`set csrConfiguration ~= (_.withClasspathOrder(false))` but I think
it's better that the test actually reflect the expected behavior
until I push the fix.
3. auto-plugins there was a hack that seemed added to address
https://github.com/sbt/sbt/issues/3164. I cannot tell from either the
issue or the linked PR what was going on and since removing the
lines that were explicitly commented as being temporary fixed it,
I figured it was ok to remove them.
This reverts commit e01f5f5ef1.
Pending tests really slow down scripted runs because they tend often
cause sbt to exit which means the next test has to reload the whole
build instead of just reloading. Unfortunately cold loading is still
pretty slow so this is significant overhead.
Before disabling these, the dependency-management suite took 335 seconds
on my computer. After, it dropped to 280 seconds.
It is expensive to compute the the hash of every jar on the classpath so
we can try to avoid that by using the timeWrappedStamper which only
computes the hash if the last modified time has changed.
Using the managedCached introduced an unintended performance regression
because it ensured that we always computed the hash of each jar on the
dependency classpath. The backing ReadStamps only computes the stamp if
the timestamp of the jar has changed.
There were a number of unused key lint warnings when loading the sbt
build. In the case of `fork in compile` and `crossVersion in update`, it
wasn't clear that these were actually used, so I removed those settings.
The others seemed to be used so I just added them to the exclude list.
To make this work with legacy versions of sbt, I redefined the
excludeLintKeys key. Once we update the build.properties to a 1.4.x
version, we can drop the `val excludeLint` definition and replace
`excludeLint` with `excludeLintKeys`.
Side note: ++= does not work with excludeLintKeys which is why I used +=
for the excludes.
I noticed that if lintUnusedKeysOnLoad := true is set, it emits a lint
warning.
As a side note, Project linting takes about 300-400ms in the sbt project
so we might want to consider disabling it by default in batch mode at
least.
The sbt project load made a number of relatively inefficient
transformations of scala collecitons. I went through and found the slow
parts during project loading and made my best attempt at fixing them.
The most significant changes I made were in places using IMap. An IMap
is more or less a wrapper around an immutable Map. It can be much faster
to construct an IMap by creating a java mutable hashmap, wrapping it a
scala Map that delegates to the underlying java hashmap (with a copy on
write if the map is modified) and constructing the IMap from the wrapped
map. It was also in many cases to parallelize some transformations
wherever the order didn't matter.
After applying all of these changes, I found that loading the sbt
project took generally between 8.5 and 9 seconds on my laptop. With
1.3.13, it hovered around 11.5 seconds. I saw a similar speedup in zinc.
The biggest specific improvement was that generating the compiled map
dropped from between 3.5-4 seconds to pretty consistently being around
1.5 seconds.
The more we call flush, the more likely progress status is to blink. To
reduce the amount of calls to flush, we can instead batch all of the
bytes that are going to be written and wriite them all at once. This
change made progress noticeably less blinky in the zinc project running
the latest sbt snapshot (which frankly was almost siezure inducing when
running publishLocal)
Printing a new line was not great ux. You might see something like:
[info] set current project to project (in build file:project)
sbt:project>
[info] new client connected: network-1
sbt:project>
instead of initially
[info] set current project to project (in build file:project)
sbt:project>
and then after the client connects:
[info] set current project to project (in build file:project)
[info] new client connected: network-1
sbt:project>
This reverts commit b1dcf031a5.
I found that b1dcf031a5 had some
unintended consequences that seemed to mess up the prompt state. The
real problem that it was trying to address was that the prompt was being
interleaved with log messages in some scenarios. There was a different
way to fix that in ProgressState that was both simpler and more
reliable.
The jline2 history file format is incompatible with jline3 and jline3
prints a very noisy warning if it detects such a file. History will also
not work with jline3 until you remove or reformat the old file.
I noticed that when reloading the build, that certain errors are logged
by sbt to System.err. These were not shown to a thin client because we
weren't forwarding System.err. This change remedies that.
System.err is handled more simply than System.out. We do not put
System.err through the progress state because generally System.err is
tends to be unbuffered. I had hesitated to add System.err to the
Terminal interface at all to give users an escape hatch but I couldn't
get project loading to work well with the thin client without it.
In db4878c786, TaskProgress became an
object which mostly made things easier to reason about. The one problem
was that it started leaking tasks with every run because the timings map
would accumulate tasks that weren't cleared. To fix this, we can clear
the timings and activeTasksMap in the TaskProgress object in the
afterAllCompleted callback. Some extra null checks needed to be added
since it's possible for the maps to not contain a previously added key
after reset has been called.
This fixes the nio/external-hooks test and also restores the performance
of the benchmarks for the latest sbt version in
https://github.com/eatkins/scala-build-watch-performance which had
regressed when the custom ExternalHooks were disabled in
7c4b01d9f7.
The main change is that it changes the ReadStamps object that is passed
into the compiler options to one that uses the unmanagedFileStampCache
and managedFileStampCache for source files and falls back to the default
stamper otherwise. This improves the performance quite significantly
since we only hash the files once. It also makes it so that the analysis
file will contain the source file stamps of the files when compilation
began, rather than when compilation ended. That is what
nio/external-hooks was testing. In the real world what could happen was
that one modified a source file during compilation but then no
incremental re-compilation would occur because after the initial
compilation completed, zinc wrote the stamp of the modified source file
in the analysis file even though it may have actually compiled a
different version of the source file.
I noticed some RejectedExecutionExceptions in travis failures of
ClientTest. This could happen if we try to write output to the
network channel after it has been closed. To avoid this problem, we can
catch RejectedExecutionExceptions and do an immediate flush if the
executor has been shutdown.
Ethan Atkins
eugene yokota
adpi2
Eric Peters
Jacob Wang