Request for permission to fix SI-8990

[Marc Siegel]

Hi Scala community,

Thanks for all your excellent work on this great language. A few weeks ago, I posted a bug about the standard library in Scala:

https://issues.scala-lang.org/browse/SI-8990

I'd like to take a crack at fixing this, obviously with a failing test first. However, following the protocol for pull requests [1], I am first writing to ask for formal permission before making such a pull request. 

I'd also like to note, as I am currently sitting at NEScala 2015 listening to Dick Wall's talk "Scala Needs YOU", that this seems like an unnecessarily hurdle -- couldn't we just discuss feedback on such a pull request?

In any case, I await your comments, and stand ready to contribute my first bug fix to the Scala library if deemed appropriate.

Warm regards,

-Marc

[1] http://docs.scala-lang.org/scala/pull-request-policy.html

[Rex Kerr]

Sure, go for it!

Or assign it to me and I'll fix it this weekend (if you're allowed to assign it).

The reason to ask on the mailing list is to avoid duplicated or wasted effort.  If you don't mind plowing ahead and having some PRs rejected, then IMO just plow ahead.

  --Rex

--
You received this message because you are subscribed to the Google Groups "scala-internals" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Adriaan Moors]

Hi Marc,

Welcome! The wording of the policy is not the greatest (I believe Dick has a spring cleaning on his TODO list)...

As Rex implied, it's perhaps a bit to optimistic in its concern for duplicated work :-)

In any case, we'll be more than happy to discuss your contribution over at the pull request buffet!

cheers

adriaan

[Jason Zaugg]

We're also trying to save people from spending a bunch of time on a patch that we won't be able to accept for some reason that might become apparent with a quick discussion. ("last time we tried that, we found..." / "fixing thing A will break thing B"). It can also be useful to discuss a design before diving into implementation. 

But we should definitely reword the guide to reduce friction for smaller contribs where the cost of this coordination outweighs the benefits. 

Jason 

—
Sent from Mailbox

[Marc Siegel]

Got it, ok that does make sense, maybe an exception for small fixes should be there.

However, I am having a moment of doubt that this is a "small" fix.

Because Stream#StreamWithFilter extends TraversableLike#WithFilter, it seems that it is closing over a strong reference to the head of the original stream just by creation. I have a test that demonstrates this -- that simply instantiating a StreamWithFilter prevents that stream from being collected even in other methods.

Does anyone have any thoughts on the best way to allow a StreamWithFilter to not retain a strong reference to head of the original stream?

Regards,

-Marc

[Marc Siegel]

Hi guys, do you have a moment to review proposed fix here?

I know it is marked as failing [1], but that is a change in binary compatibility that I think we should discuss. The current return type of Stream#withFilter, as documented in the PR, *must* change in order to fix this, and the proposed new return type is an implementation of FilterMonadic, which should fit the expectations of any conceivable use case, right?

[1] https://scala-ci.typesafe.com/job/scala-2.11.x-validate-test/61/console

[Marc Siegel]

Hi all, the latest PR to fix SI-8990 targetted to 2.12.x is here: https://github.com/scala/scala/pull/4284

Thanks,

-Marc

[Marc Siegel]

Why is the CI bot running this PR against 2.11.x? What is going on here?

https://scala-ci.typesafe.com/job/scala-2.11.x-validate-publish-core/308/

[Marc Siegel]

Ah got it, nevermind, re-pushing.

(2.12.x introduced a test class with the name StreamTest, I needed to merge them after rebasing)

[Marc Siegel]

Hi all,

This is a bit frustrating -- can anyone explain what, if anything, these test failures signify?

https://scala-ci.typesafe.com/job/scala-2.11.x-validate-main/279/

https://scala-ci.typesafe.com/job/scala-2.11.x-integrate-ide/50/

https://scala-ci.typesafe.com/job/scala-2.11.x-validate-test/72/

As far as I can tell, the tests actually pass, so need some more knowledgeable assistance here.

/CC Dick Wall as this does seem a barrier to entry for contributing a fix ;)

Thanks,

-Marc

[Jason Zaugg]

Hi Marc,

A unit test is failing in the `validate-test` job. Looking at its Console Output:

BUILD FAILED
/home/jenkins/workspace/scala-2.11.x-validate-test@2/build.xml:1478: Test scala.collection.immutable.StreamTest failed

We've recently moved to a new Jenkins farm for pull request validation, and it appears that we have lost the configuration that used to archive the individual JUnit test reports which would contain the exception or failed assertion. I've opened an issue to make sure we restore that.

You can run this test from the command line with ant test.junit -Dtest.class scala.collection.immutable.StreamTest.

​

(I see you have figured this part out on the PR discussion. I'll send this anyway as it might be generally useful.)

-jason

[Marc Siegel]

Hi Jason, I'm not sure it's that simple.

While modifying `Stream.scala`, the behavior of the test doesn't always change, unless I also modify `StreamTest.scala`, when using `source scripts/jobs/validate/test` to exactly replicate CI failure.

However, when I use `ant test.junit -Dtest.class scala.collection.immutable.StreamTest`, and I modify `Stream.scala`, I often get the cryptic error:

quick.lib:

[quick.library] Compiling 1 file to /Users/msiegel/Documents/workspace/OpenSource/scala/build/quick/classes/library

[quick.library] Error: Could not find or load main class scala.tools.nsc.Main

BUILD FAILED

/Users/msiegel/Documents/workspace/OpenSource/scala/build.xml:1110: The following error occurred while executing this line:

/Users/msiegel/Documents/workspace/OpenSource/scala/build-ant-macros.xml:275: The following error occurred while executing this line:

/Users/msiegel/Documents/workspace/OpenSource/scala/build-ant-macros.xml:284: The following error occurred while executing this line:

/Users/msiegel/Documents/workspace/OpenSource/scala/build-ant-macros.xml:227: Compilation failed because of an internal compiler error; see the error output for details.

So what I am having to do is to modify a println in the library; modify a println in the test; and run the full validate script, and then verify in the test output file that I see the latest printlns.

Can you reproduce that behavior?

-Marc 

[Marc Siegel]

Back to the actual issue (SI-8990): How would everybody feel if `Stream#withFilter` simply invoked `Stream#filter` directly? Since Stream is already a lazy data structure, in other words, is there any need to have a separate implementation of withFilter, which I understand only to exist in order to make for-comprehensions lazy for strict data structures?

If there is appetite for this, I have verified that solving the problem that way does fix this bug, at the cost of eagerly filtering out all non-matching elements at the head of the stream.

Thoughts?

-Marc

[Rex Kerr]

I don't see why the especially-lazy version shouldn't work, though.  We ought to get to the bottom of this (by looking at bytecode if nothing else suffices) instead of adopting alternate behavior for reasons we don't fully understand.

Which is your best attempt that ought to work yet fails when run on some supported JVM?  (And which one, exactly, did you use to reproduce it?)

You seem to have ended up with an unusually and unexpectedly difficult case for a first patch!

  --Rex

[Adriaan Moors]

My intuition is that withFilter is an optimization that's part of the contract, and thus is required:

   *  Note: the difference between `c filter p` and `c withFilter p` is that

   *        the former creates a new collection, whereas the latter only

   *        restricts the domain of subsequent `map`, `flatMap`, `foreach`,

   *        and `withFilter` operations.

We shouldn't break the promise of no new collection being created.

[Marc Siegel]

Ok Rex and Adriaan, I just wanted to float the idea (as I know it works), didn't expect that idea to catch on.

Please look at the latest version of this PR: https://github.com/scala/scala/pull/4284

You will notice that sometimes StreamTest#withFilter_foreach_allowsGC will pass, and sometimes it will fail.

To get differing behavior, I think you need to chance the StreamTest.scala to cause a recompile, but I am not certain. I usually increment a println.

I looked at bytecode and also at a heapdump, and in both cases I see that the null-after-use pattern has removed the reference to the original head of the stream, and in fact in the jvisualvm heapdump I can see that there are no references left to it except for the WeakReference in the test.

Therefore, I need help understanding why sometimes it is not allowed to be collected?

Thanks very kindly,

-Marc

[Rex Kerr]

I wonder if it is actually the WeakReference itself which is somehow not getting cleared?  The JVM could have been tweaked to make it less or more or differently aggressive about cleaning them up.  It certainly doesn't promise to do it after a single call to gc().

Som-snytt had a test that really pushed the JVM hard to give up the memory.  Maybe you should use that one.  It seemed pretty reliable.

  --Rex

[Marc Siegel]

Hi Rex,

I wonder if it is actually the WeakReference itself which is somehow not getting cleared

While totally sensible, this idea is disproved by the nearly identical tests in StreamTest.scala (foreach_allows_GC and filter_foreach_allows_GC), which both always pass.

No, I think there is something about calling .withFilter, which must return an object on which the next call is made. There appears to be cases where this results in a reference on the stack that somehow prevents collections, and cases where it does not. I'm happy to screen share or otherwise remote pair, btw, if someone is interested in getting to the bottom of this?

Regards,

-Marc

[Marc Siegel]

Hi Rex and Adriaan, if you take the time to run the StreamTest in my PR in IntelliJ, guess what: the tests pass *every* time. So perhaps it has something to do with the JAVA_OPTS or perhaps the flags passed to ant in scripts/jobs/validate/test [1]?

[1]

ant -Dstarr.version=$scalaVersion \

       -Dscalac.args.optimise=-optimise \

       -Dlocker.skip=1 -Dstarr.use.released=1 -Dextra.repo.url=$prRepoUrl \

       $testExtraArgs ${testTarget-test.core docs.done}

Again, any insight into how this different behavior can manifest would be appreciated!

Thanks,

-Marc

[Rex Kerr]

Can you just describe what setup you use to see the inconsistency in GC (or loss of GC)?  Both filter and withFilter create new objects.  That shouldn't be the critical difference.

The solution is probably the same either way: push the JVM hard to want to clean up memory.  It has to care enough to recover the memory for the stream, and it has to care enough to null out the weak reference.  It doesn't promise when it'll do either.

I'm happy to take a look myself if I know exactly what I should look at.

Incidentally, if you make a stream with vast memory requirements (e.g. 1M length stream containing a 100k array at each spot), do you get an OOME under those conditions where the tests fail?

  --Rex

[Marc Siegel]

Hi Rex, 

Can you just describe what setup you use to see the inconsistency in GC (or loss of GC)?  Both filter and withFilter create new objects.  That shouldn't be the critical difference.

My apologies if this hasn't been clear. I've tried hard to make it very clear what setups I've tried, which manifest failing under what circumstances, and what the different behavior of the different tests might prove. I'm happy to discuss via a voice or video mechanism as I think a quick chat will be easier than the back and forth?

I've discussed in previous emails the following setups:

  a.  Exactly reproduce the CI environment

        - set JAVA_OPTS, MAVEN_OPTS, ANT_OPTS, scalaVersion, etc, from the scala-jenkins project

        - run `git clean -fdx` to ensure all rebuilt

        - run `source scripts/jobs/validate/test` just like CI

  b.  Via ant

        - run `ant test.junit -Dtest.class scala.collection.immutable.StreamTest`

  c.  Via IntelliJ 14

        - follow instructions in src/intellij-14/README

        - run test by right-clicking on StreamTest and clicking "Run 'StreamTest'"

Does that clarify? All the code I am discussing you can get by checking out the PR: https://github.com/scala/scala/pull/4284

The solution is probably the same either way: push the JVM hard to want to clean up memory.  It has to care enough to recover the memory for the stream, and it has to care enough to null out the weak reference.  It doesn't promise when it'll do either.

 
I'm happy to take a look myself if I know exactly what I should look at.

 

Incidentally, if you make a stream with vast memory requirements (e.g. 1M length stream containing a 100k array at each spot), do you get an OOME under those conditions where the tests fail? 

It's certainly worth another shot! Although I don't understand why the other two tests would always pass if that was the issue, since they should be stressing the JVM in exactly the same way, with the same code, in order to disprove that idea. As you can see in the latest commit in my PR, I have removed the `.take(500)` code, so now the streams are infinite. If they don't terminate when the head is collected, they should result in OOM or timeout now: https://github.com/ms-tg/scala/commit/f894c55ef3789cd9d694ce29b4c4c792dd4fd2ff

Thanks again,

-Marc

[Adriaan Moors]

Thanks for persevering, Marc!

Maybe it's one of the crazy options in our JAVA_OPTS? What happens when you run IntelliJ using them? Can you narrow it down to a single culprit? -XX:+UseParNewGC perhaps?

From https://github.com/scala/scala-jenkins-infra/blob/master/attributes/master.rb#L28:

JAVA_OPTS="-Dfile.encoding=UTF-8 -server -XX:+AggressiveOpts -XX:+UseParNewGC -Xmx2G -Xss1M -XX:MaxPermSize=512M -XX:ReservedCodeCacheSize=128M -Dpartest.threads=4"

--

You received this message because you are subscribed to the Google Groups "scala-internals" group.

[Scott Carey]

On Wednesday, February 4, 2015 at 1:34:56 PM UTC-8, Adriaan Moors wrote:

Thanks for persevering, Marc!

Maybe it's one of the crazy options in our JAVA_OPTS? What happens when you run IntelliJ using them? Can you narrow it down to a single culprit? -XX:+UseParNewGC perhaps?

From https://github.com/scala/scala-jenkins-infra/blob/master/attributes/master.rb#L28:

JAVA_OPTS="-Dfile.encoding=UTF-8 -server -XX:+AggressiveOpts -XX:+UseParNewGC -Xmx2G -Xss1M -XX:MaxPermSize=512M -XX:ReservedCodeCacheSize=128M -Dpartest.threads=4"

Hmm, on some JVM versions, AggressiveOpts enables extra escape analysis.  That is a moving target bucket of extra stuff that will vary from JVM to JVM and could result in inconsistent JIT results.

One could try logging the JIT compilation and see if the failure cases JIT differently -- perhaps inlining differently.
 

[Marc Siegel]

Ok guys, I will examine the JAVA_OPTS. I think maybe this will work.

As you can see, although it succeeded locally (without all the JAVA_OPTS), it failed to work on CI:

    https://scala-ci.typesafe.com/job/scala-2.11.x-validate-test/96/console

Thanks,

-Marc

[Marc Siegel]

Well, it's not the JAVA_OPTS, and it's not the size of the memory pressure. I've exhaustively experimented with both.

I hypothesize that the difference may actually be in the Scala compiler. The code and tests as present in my PR appear work fine in IntelliJ, which is actually using the 2.11.x starr compiler. And I believe that when I recompile tests with `ant test.junit`, again I think I may be getting the starr compiler.

Can anyone comment on that?

Thanks,

-Marc

[Marc Siegel]

Ah yes, here are the steps to reproduce:

# Get code from PR

git clone https://github.com/ms-tg/scala.git pr-4284

cd pr-4284

git checkout SI-8990-rebase-on-2.12-squashed 

git clean -fdx

# Reproduce CI environment

export JAVA_OPTS="-Dfile.encoding=UTF-8 -server -XX:+AggressiveOpts -XX:+UseParNewGC -Xmx2G -Xss1M -XX:MaxPermSize=512M -XX:ReservedCodeCacheSize=128M -Dpartest.threads=4"

export ANT_OPTS="-Dfile.encoding=UTF-8 -server -XX:+AggressiveOpts -XX:+UseParNewGC -Xmx2G -Xss1M -XX:MaxPermSize=512M -XX:ReservedCodeCacheSize=128M -Dpartest.threads=4"

export MAVEN_OPTS="-Dfile.encoding=UTF-8 -server -XX:+AggressiveOpts -XX:+UseParNewGC -Xmx2G -Xss1M -XX:MaxPermSize=512M -XX:ReservedCodeCacheSize=128M"

export prRepoUrl="http://private-repo.typesafe.com/typesafe/scala-pr-validation-snapshots/"

export scalaVersion="2.12.0-f894c55-SNAPSHOT"

# Run tests as they are run in CI on 2.12.x

source scripts/jobs/validate/test                                 # eventually fails, or kill it first

ant test.junit -Dtest.class scala.collection.immutable.StreamTest # NOTICE that this also fails here

# Change some line in StreamTest.scala and re-run ant

## change something in the test code

ant test.junit -Dtest.class scala.collection.immutable.StreamTest # NOTICE that this now passes

So what does this mean? Is this behavior a possible regression the Scala compiler, since I can't actually find any reason that this code should not work, and it works on 2.11.x?

Thanks again for any help,

-Marc

[Jason Zaugg]

On Thu, Feb 5, 2015 at 12:33 PM, Marc Siegel <marc....@timgroup.com> wrote:

Ah yes, here are the steps to reproduce:

# Get code from PR

git clone https://github.com/ms-tg/scala.git pr-4284

cd pr-4284

git checkout SI-8990-rebase-on-2.12-squashed 

git clean -fdx

# Reproduce CI environment

export JAVA_OPTS="-Dfile.encoding=UTF-8 -server -XX:+AggressiveOpts -XX:+UseParNewGC -Xmx2G -Xss1M -XX:MaxPermSize=512M -XX:ReservedCodeCacheSize=128M -Dpartest.threads=4"

export ANT_OPTS="-Dfile.encoding=UTF-8 -server -XX:+AggressiveOpts -XX:+UseParNewGC -Xmx2G -Xss1M -XX:MaxPermSize=512M -XX:ReservedCodeCacheSize=128M -Dpartest.threads=4"

export MAVEN_OPTS="-Dfile.encoding=UTF-8 -server -XX:+AggressiveOpts -XX:+UseParNewGC -Xmx2G -Xss1M -XX:MaxPermSize=512M -XX:ReservedCodeCacheSize=128M"

export prRepoUrl="http://private-repo.typesafe.com/typesafe/scala-pr-validation-snapshots/"

export scalaVersion="2.12.0-f894c55-SNAPSHOT"

# Run tests as they are run in CI on 2.12.x

source scripts/jobs/validate/test                                 # eventually fails, or kill it first

ant test.junit -Dtest.class scala.collection.immutable.StreamTest # NOTICE that this also fails here

# Change some line in StreamTest.scala and re-run ant

## change something in the test code

ant test.junit -Dtest.class scala.collection.immutable.StreamTest # NOTICE that this now passes

So what does this mean? Is this behavior a possible regression the Scala compiler, since I can't actually find any reason that this code should not work, and it works on 2.11.x?

My hunch is that you are witnessing a difference between the regular and optimizing Scala compiler. The optimizer might inline something in StreamTest that was material to the collectability.

The target test.junit depends on quick.done, rather than quick-opt which will be used by the release and PR validation scripts.

Try:

% touch test/junit/scala/collection/immutable/StreamTest.scala

% ant build-opt # optimized

% echo ':javap -c -private scala.collection.immutable.StreamTest' | qscala -classpath > opt.javap

% touch test/junit/scala/collection/immutable/StreamTest.scala

% ant build # unoptimized

% echo ':javap -c -private scala.collection.immutable.StreamTest' | qscala -classpath > regular.javap

% diff -u {regular,opt}.javap

​

Hope this helps!

-jason

[Marc Siegel]

Thanks for this Jason. I'd like to try out the commands you provided, but I don't have `qscala`, and it seems non-googlable. Can you direct me to it?

Thanks,

-Marc

[Lukas Rytz]

Hi,

this is just Jason's alias to the `scala` executable in his checkout, i.e., when you run "ant build" on the scala repo, you get build/quick/bin/[scala|scalac] and build/pack/bin/[scala|scalac]. The two are the same, except that one runs on the classfiles in your file system (build/quick), the other one on the packed jar of those (build/pack).

Lukas

--

[Adriaan Moors]

Here's some more of the tooling we use:

https://github.com/retronym/libscala

https://github.com/adriaanm/binfu (a dump of my bin directory -- not terribly thrilling)

[Marc Siegel]

Hi Adriaan and Lukas,

Thank you for your help and being patient with me.

I find it frustrating that none of these replies and project links actually write out the `alias qscala=...` line. Is there a reason for that? (/cc Dick Walls)

Also of note: libscala.sh is one of many references to qscala and qscalap that show up in Google, but of course no actual definition of it.

-Marc

[Jason Zaugg]

On Fri, Feb 6, 2015 at 9:26 AM, Marc Siegel <marc....@timgroup.com> wrote:

Thank you for your help and being patient with me.

I find it frustrating that none of these replies and project links actually write out the `alias qscala=...` line. Is there a reason for that? (/cc Dick Walls)

Also of note: libscala.sh is one of many references to qscala and qscalap that show up in Google, but of course no actual definition of it.

Ah, looks like I have them defined locally as:

% type qscala
qscala is a function
qscala ()
{
    $(git rev-parse --show-toplevel)/build/quick/bin/scala -nc "$@"
}

% type qscalac
qscalac is a function
qscalac ()
{
    $(git rev-parse --show-toplevel)/build/quick/bin/scalac "$@"
}

​

-jason

[Som Snytt]

There's also a case to be made against over-complication.

When starting out, you really just want to make a distro, even if takes longer. After a few of those, you have time to become more efficient.

I remember that the old Readme for partest was very discouraging. I just wanted to run tests.

Also, maybe it's easier on a Swiss keyboard, but qscalac is not easy for me to type.

--

[Marc Siegel]

Update: After the @noinline fix suggested by Jason, and my fix to his suggested code to allow retry on exception during filtering, the tests now pass on 2.12.x:

https://github.com/scala/scala/pull/4284

Hoping everyone can review again now?

Thanks,

-Marc

[Marc Siegel]

Update: Per discussion on the PR, Rex Kerr has volunteered to review in detail, so ball is in his court.

Thanks,

-Marc

[Marc Siegel]

Thanks to Rex, Adriaan, Jason, and everyone else who helped with this. The fix for SI-8990 has been pulled upstream now.

Pursuant to discussion of SI-9134 (https://issues.scala-lang.org/browse/SI-9134), I've submitted a PR of two tests demonstrating that it is now fixed:

    https://github.com/scala/scala/pull/4362

Does anyone want to take a look at this?

Thanks,

-Marc