Skip to content

Speed up partest #6391

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

Closed
wants to merge 6 commits into from
Closed

Speed up partest #6391

wants to merge 6 commits into from

Conversation

retronym
Copy link
Member

@retronym retronym commented Mar 7, 2018

Prepares for, and then switches to, partest 1.1.7 which
includes a mode to run tests without forking.

Removes some resource hungry tests. Volunteers sought to revive
these in more frugal form.

retronym added 3 commits March 7, 2018 22:44
 - Explicitly add test output path to the compiler classpath
   Rather than assuming its in the application classpath.
 - Ensure tests cleanup threads
 - Fork tests that inherently require it or ones that I can't
   figure out how to make work without it
Tests that take 30+ seconds to compile and/or execute
really add up to a productivity loss for contributors.

These tests served a purpose to show that the corresponding
changes were correct. But if we want them to serve an ongoing
purpose of guarding against regressions, they need to be
maintained to do so more quicky or be moved into a test suite
that is run less frequently.
@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Mar 7, 2018
@retronym
Copy link
Member Author

retronym commented Mar 7, 2018

The last two commits won't build until scala/scala-partest#99 is reviewed, merged, and released in Partest 1.1.6. My intention is that the prior commits should build successfully: the changes to the tests should be made in a backwards compatible way. In other words, all tests must support forked execution.

@retronym
Copy link
Member Author

retronym commented Mar 7, 2018

To test out this change, you need to:

  • checkout retronym:ticket/75 in scala-partest
  • run sbt 'set every version := "1.1.6-pre-LOCAL1"' '++2.13.0-M3' ';clean;publishLocal'
  • checkout retronym:topic/partest-apace of scala
  • run sbt -Dpartest.version=1.1.6-pre-LOCAL1 'partest --pos --neg --run --jvm --specialized --presentation --instrumented'

Things to test: the test failure modes all still work (output differs from checkfile, sys.exit(1) or throw new Throwable in a test, compilation error.

@@ -1,269 +0,0 @@
object foo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I had been debating removing this from about a day after I introduced it, but I didn't want to make a "delete a test" PR.

@retronym
Copy link
Member Author

retronym commented Mar 8, 2018

/rebuild

@retronym
Copy link
Member Author

retronym commented Mar 8, 2018

I rebuilt to measure the best-case timing on an uncontended worker. (The first build landed in the third and final executor slot a worker already building another validate-main job and the community build). The rebuild took 24 minutes (note that rebuilds take a shortcut by skipping a previously successful publish-core step which saves about 4 minutes).

We've still got a race condition in our System property handling:

	... 4 more
Caused by: sbt.ForkMain$ForkError: sbt.ForkMain$ForkError: java.util.ConcurrentModificationException: null
	at java.util.Hashtable$Enumerator.next(Hashtable.java:1383)
	at scala.collection.convert.Wrappers$JPropertiesWrapper$$anon$3.next(Wrappers.scala:420)
	at scala.collection.convert.Wrappers$JPropertiesWrapper$$anon$3.next(Wrappers.scala:416)
	at scala.collection.TraversableOnce.collectFirst(TraversableOnce.scala:139)
	at scala.collection.TraversableOnce.collectFirst$(TraversableOnce.scala:126)
	at scala.collection.AbstractTraversable.collectFirst(Traversable.scala:104)
	at scala.tools.util.PathResolver$Environment$.searchForBootClasspath(PathResolver.scala:53

Another thread in another part of partest, which is compiling files, is iterating through the System properties and detects a concurrent modification.

I saw that locally, and thought I'd fixed it by avoiding mutating the System property map in place, and instead swapping in a brand new, fully built Properties.

https://github.com/scala/scala-partest/blob/09d2dcc549235e88fac38be1670974d843cc887f/src/main/scala/scala/tools/partest/nest/StreamCapture.scala#L41-L52

But looking at this again, there is no safe publication, so the other thread could see a stale value for Hashtable.modcount when iteration starts, and only during iteration get the latest value after a cache line has been flushed. Maybe I can fix that with a memory barrier. I'd rather not have to serialize execution and compilation.

@retronym retronym force-pushed the topic/partest-apace branch from 70239c1 to 318821d Compare March 8, 2018 03:05
@retronym
Copy link
Member Author

retronym commented Mar 8, 2018

In addition to trying to use Unsafe.storeFence within Partest 1.1.7 (which passed the build in 8a2490c, although that doesn't prove the race is ruled out), I've also changed the way we iterate through the System properties in the compiler (c9398d0) to be more robust in the face of mutation from other threads.

@lrytz
Copy link
Member

lrytz commented Mar 12, 2018

This should go in 2.12.x eventually - why did you pick 2.13.x for this PR?

@retronym retronym mentioned this pull request Mar 12, 2018
@retronym
Copy link
Member Author

Originally I assumed there was more divergence between the partest versions used on 2.12 and 2.13, but that appears not to be a problem.

I've rebased this PR as #6413 to target 2.12.x, we can see how it fares. Probably best to defer until after the 2.12.5 release to be on the safe side.

@retronym
Copy link
Member Author

I also needed to backport 9c7362b to 2.12.x.

@retronym
Copy link
Member Author

Closing in favour of the 2.12.x targetted PR, #6413

@retronym retronym closed this Mar 21, 2018
@SethTisue SethTisue removed this from the 2.13.0-M4 milestone May 4, 2018
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.

5 participants