Restructuring repo and issues #1636
Conversation
| ";dotty-library/package" + | ||
| ";dotty-compiler/test:package" | ||
| ) ++ | ||
| addCommandAlias( | ||
| "partest", | ||
| ";packageAll" + |
There was a problem hiding this comment.
I guess that then ;packageAll can be removed from all the command aliases :)
There was a problem hiding this comment.
Probably, yes. (Also I tried using a Sequential Task for packageAll but when I ran it, it did nothing, any idea why?)
There was a problem hiding this comment.
Don't use Def.sequential unless you know exactly what you are doing.
Def.sequential does not give much guarantee in terms of sequentiality. In particular, if from X you depend on task A which depends on a sequence of B, C; but at the same time X depends on C (via another path of dependencies); then B and C will probably still run in parallel.
| @@ -12,7 +12,7 @@ object DottyBuild extends Build { | |||
| val baseVersion = "0.1" | |||
| val isNightly = sys.env.get("NIGHTLYBUILD") == Some("yes") | |||
|
|
|||
| val jenkinsMemLimit = List("-Xmx1300m") | |||
| val jenkinsMemLimit = List("-Xmx1500m") | |||
There was a problem hiding this comment.
why is more memory needed after those changes?
There was a problem hiding this comment.
I think the commit message explains it.
There was a problem hiding this comment.
Would you mind explaining the bug?
7f4f387
does a lot of changes but I don't see anything particularly related to -deep or something similar.
The biggest tests that we had so far were stdlib and dotty. Stdlib relies on whitelist, and dotty did use -deep flag and they did run fine with previous heap size. If this flag works unreliably, it would be nice to know as other forks depend on it.
There was a problem hiding this comment.
The bugfix is at: 7f4f387#diff-ca99a4570465b856d9f97804352f6851R167
compare with the implementation of sources in https://github.com/scala/scala-partest/blob/7c265b38352f146f941a873b153cbfc22d7b7afd/src/main/scala/scala/tools/partest/nest/Runner.scala#L427
There was a problem hiding this comment.
The question still persists. This fix only changes behaviour of partest, the bug wasn't present in jUnit test suite. Why does partest need substantially more memory to run(compared to jUnit tests)?
I'm sure that previous September we were able to compile dotty & stdlib using only 750mb of memory without GC-related issues(i had to work on a very slow machine that had 2GB of memory for a week and I've decreased -Xmx to 750mb). In March, you acf0535 have bumped the upper limit to 1.3Gb. Now we bump it to 1.5GB.
Since last September, tests didn't grow much. Yes, dotty is now 20% bigger, white collection whitelist is negligibly bigger. Tests do not explain why dotty now needs at least twice more memory to pass the test suite, compared to situation 1 year ago.
There was a problem hiding this comment.
Previously dotty was not compiled properly by partest. It was only able to compile top-level files (since the compiler itself was on the class path).
It never recursed obeying -deep.
EDIT: was on my phone, now I have a computer, here's a better explanation: previously what happened was that partest copied files (correctly, with deep) to the partest-generated directory. A flags file was created with the correct flags, then partest would do a set up and call the method sources to get the sources for which to do the compilation with DPDirectCompiler. This did not work as intended. Instead of getting all the sources or just giving the compiler a directory, it would give the compiler the specific sources that were at the top-level of the sources directory - for dotty that would mean DottyPredef.scala (maybe more historically).
The partest suite would in the cases where there was no scala or java files in the top dir actually do ~sources(dir).tail and throw an exception. This exception was entirely swallowed by the suite and didn't show until printlns were added to an absurd extent. Once we uncovered this behavior, we could verify that pos/repl was only compiling the top-level files and not the files contained within the sub dir ammonite.
So, since partest never actually compiled the whole of dotty in parallel with other tests, we probably don't have a very exact baseline for what the suite needs - that is why @smarter increased the heap-size IIUC.
There was a problem hiding this comment.
@felixmulder, I do understand that partest did the wrong thing. What I mean is that the current situation indicates that we use too much memory. Note, that scalac runs the entire test suite in parallel under partest under -Xmx1024m(that's what is used by CI AFAIK).
The baselines, that seem reasonable to me are:
- normal hello-world like tests shouldn't need more than 100mb of memory.
- dotty & stdlib tests shouldn't need more than 0.5 gigs of memory.
Assuming our CI runs 4 compiler threads, this means that we would never need more than 1.2 Gb, and we'll only need 1.2 Gb if we're unlucky and dotty & stlib are compiled concurrently. If you'll have a look at the log output of the build failed with GC https://scala-ci.typesafe.com/job/dotty-master-validate-partest/2417/consoleFull, you'll see that these two tests did not run concurrently.
There was a problem hiding this comment.
Yes, please open an issue so that we can track this.
|
Remaining issues:
|
|
Yay, now the unit tests are passing again. Hopefully it's just that pesky stale symbol left now. |
felixmulder
force-pushed
the
topic/restructure-attempt2
branch
6 times, most recently
from
6797cb5 to
31c1900
Compare
| @@ -180,6 +181,28 @@ object DottyBuild extends Build { | |||
| ) | |||
| }.evaluated, | |||
|
|
|||
| // Override run to be able to run compiled classfiles | |||
| run := { | |||
There was a problem hiding this comment.
I think this is super confusing. In every other sbt project run means run the Main of this project but not here, I'm fine with having a dotr command but run should keep being the actual sbt run
There was a problem hiding this comment.
I thought you would say that - but waited until you did. I'll patch it.
felixmulder
force-pushed
the
topic/restructure-attempt2
branch
5 times, most recently
from
22a093c to
c53a5e9
Compare
smarter
force-pushed
the
topic/restructure-attempt2
branch
from
c53a5e9 to
a05bc05
Compare
|
Rebased (more commits to come). |
smarter
force-pushed
the
topic/restructure-attempt2
branch
3 times, most recently
from
f5da246 to
691aa97
Compare
|
@felixmulder: Remaining known issues:
@odersky : Commits you may want to review: |
|
@felixmulder Also it would be really nice if |
This might cause a stale symbol exception and make it harder to find the source of the problem.
Previously Outer2#Inner#foo failed to compile with:
```
non-private method foo refers to private value x in its type signature
```
This should compile because the boundary of `foo` is `class Outer2`
and the boundary of `x` is `object Outer2`. This commit fixes this by
also considering the linked boundary in `checkNoPrivateLeaks`.
The access boundary of a def in a local class is the owner of that class, previously it was set to the access boundary of the owner of the class instead.
This could be abstracted better but we'll end up replacing checkNoPrivateLeaks soon anyway due to scala#1723
We run checkNoPrivateLeaks in the unpickler since last commit and this is causing an issue related to privacy leaks. Give up and workaround it since we're going to redesign how we handle privacy leaks because of scala#1723 anyway.
This lead to stale symbol errors in `tasty_tools` because the symbol forcing was bringing forward symbols from the previous run before the corresponding symbols for the current run were created. We fix this by adding Annotations#deferredSymAndTree which behaves similarly to Annotations#deferred but take a by-name symbol. We also remove TreeUnpickler#LazyAnnotationReader which was apparently never used.
This fix the test "Extracted source dependencies from public members"
which previously failed with:
Set('G, 'E) is not equal to Set('B, 'E) (DependencySpecification.scala:34)
`H` extends `G.T[Int]` which is an alias of `B`, so the
`topLevelInheritanceDepndencies` of `H` should contain `B`, this was not
the case before because we didn't dealias before looking for the
top-level class of the dependency, so we ended up with `G`, the
top-level class in which the alias `T` is contained.
Some tests are run with "-Ytest-pickler" which uses a huge amount of memory. By running these tests one by one when no other test is running, we avoid running out of memory.
These commands already work without defining aliases
Otherwise this would get picked up by eclipse plugin as a project named `bin`
Depending on order, scripts might choose the wrong jar (i.e. test instead of non test). This commit addresses that by sedding away results which have `javadoc` or `tests` in the jar name
Otherwise, they may end up with a qualifier, this manifested itself as a pickling difference in `tasty_tools`.
Special-casing like this is ugly, we should decide whether we want to avoid simplifications on all TypTrees and whether we want to do this just in unpickler or always. But I want to merge this PR first.
smarter
force-pushed
the
topic/restructure-attempt2
branch
3 times, most recently
from
ca447e0 to
9222af0
Compare
|
Alright, rebasing on top of master with #1725 merged seems to have helped with the remaining memory issues, now partest consistently runs in ~15 minutes. Merging now before something else breaks! :) |
Partest was not compiling everything in directories, it was compiling the top-level scala files in the directories.
Which revealed some nasty bugs. Specifically:
These previously passed because only the top-level was compiled, but now do not. Stale symbols galore.
Could I get some help on this branch?
cc: @odersky, @DarkDimius
PS - since most of these are contained within one another (dotty is a superset of repl etc) I hope it's just one bug.