SI-6412 alleviates leaks in toolboxes, attempt #2 #1393
Conversation
|
review by @paulp @gkossakowski |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/589/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1296/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1296/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/589/ |
|
PLS REBUILD ALL |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/622/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1331/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1331/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/622/ |
|
I don't know how caches work in compiler at all so I'm not qualified to review this pull request. To me this looks very dangerous. I'll wait for @paulp's opinion. |
|
LGTM. It's clear that there were leaks and the fix addresses them (probably not all of them, but a definite improvement) |
Turns importer caches into fully weak hash maps, and also applies manual cleanup to toolboxes every time they are used. It's not enough, because reflection-mem-typecheck test is still leaking at a rate of ~100kb per typecheck, but it's much better than it was before. We'll fix the rest later, after 2.10.0-final. For more information, see https://issues.scala-lang.org/browse/SI-6412 and http://groups.google.com/group/scala-internals/browse_thread/thread/eabcf3d406dab8b2 In comparison with scala@b403c1d, the original commit that implemented the fix, this one doesn't crash tests. The problem with the original commit was that it called tryFixup() before updating the cache, leading to stack overflows.
|
PLS REBUILD ALL |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/630/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1338/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1338/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/630/ |
|
So @xeno-by explained to me this change and its impact. Got a warm feeling that it affects only reflection itself and has no interactions with anything else so LGTM. Merging. |
SI-6412 alleviates leaks in toolboxes, attempt #2
4 participants
Turns importer caches into fully weak hash maps, and also applies
manual cleanup to toolboxes every time they are used.
It's not enough, because reflection-mem-typecheck test is still leaking
at a rate of ~100kb per typecheck, but it's much better than it was before.
We'll fix the rest later, after 2.10.0-final.
For more information, see https://issues.scala-lang.org/browse/SI-6412 and
http://groups.google.com/group/scala-internals/browse_thread/thread/eabcf3d406dab8b2
In comparison with b403c1d,
the original commit that implemented the fix, this one doesn't crash tests.
The problem with the original commit was that it called tryFixup() before
updating the cache, leading to stack overflows.