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

The alternative, flat representation of classpath elements #4176

Merged
merged 15 commits into from Dec 5, 2014

Conversation

Projects
None yet
7 participants
@mpociecha
Member

mpociecha commented Dec 1, 2014

This pull request provides an alternative, more efficient implementation of classpath handling in the compiler. This feature will be delivered in 2.11.5 but it's off by default. To try it out, you must enable it with YclasspathImpl:flat compiler option. The largest performance gain will be observed when multiple instances of the Scala compiler are used within the same JVM. Examples include Scala IDE (one instance per open project in Eclipse) or parallel builds in sbt. Compiler-derived tools like REPL also benefit from the same optimisations.

Below is the original description of this PR with all remaining details.


This is another PR created for the flat classpath representation. This time the target is changed to 2.11.x branch.

It's finished work started and abandoned by @gkossakowski
Related discussion: https://groups.google.com/forum/#!msg/scala-internals/xgb5YbKvQcw/7bcbsvKkzQkJ

The main goal of Grzegorz was to improve the efficiency of classpath. At the end it turned out that the efficiency improvement is not significant so Grzegorz stopped working on that.

On the other hand it was worth to continue these works due to several reasons:

  • it's possible to reduce memory consumption significantly
  • each possible efficiency improvement is important
  • the old classpath is maybe too complex because it was created when e.g. .NET was supported - it can be simpler

New (flat) classpath has dedicated classes for various file types characteristic for Scala with JVM backend - what allows us to create more efficient, more low-level operations.
To reduce memory consumption there's added caching for jar/zip files. So that we can reuse existing instances, when we create entries for the same files once again.

One of the most important things took into account during creating this implementation was adding it as an optional feature, which can be marked as experimental and turned on using flag. At the beginning users will use an old classpath implementation by default and they will be able to use the new implementation when:

  • they will want to test it (feedback)
  • they will want to gain benefits which it provides

I took into account also planned better support for JSR-223. So it supports for instance ManifestResources introduced here: #2238
There was no proper description what's going on with these changes and how to test them but @rjolly sent me some useful papers.
This particular type of classpath can be tested using e.g. command:
jrunscript -classpath scala-compiler.jar;scala-reflect.jar;scala-library.jar -l scala
where scala-library.jar was modified using https://github.com/rjolly/jarlister

I tested flat classpath in several ways:

  • First of all obviously all tests (there are added certain new ones especially for this implementation) pass and bootstrapping also works.
  • I built Scala IDE using my version of Scala and tested it with a very large codebase with about 100 projects. I turned off the closing of unused presentation compilers which I added lately. And after that I did certain custom operation which does a deep refresh of all projects (close projects, regenerate the workspace, open projects).

The flat classpath with turned on caching allowed me to reduce allocated memory in old gen from about 6GB (for current classpath) to about 750MB. So in the case of IDE and presentation compilers the gain is really significant.

  • The efficiency of both implementations in real use cases seems to be similar but probably it could be tested better. In small benchmarks the flat one was winning.
  • On the other hand there are situations when the gain is quite significant. When running a whole Scala's test suite:

Tests using a computer with SSD:

Time type: recursive classpath flat classpath
avg of 4 repetitions 29 minutes 34 seconds 27 minutes 15 seconds
min 29 minutes 28 seconds 27 minutes 6 seconds
max 29 minutes 41 seconds 27 minutes 32 seconds

Tests using other computer with the scala project directory moved to some external HDD connected via USB cable:

Time type: recursive classpath flat classpath
avg of 4 repetitions 56 minutes 43 seconds 52 minutes 38 seconds
min 55 minutes 40 seconds 52 minutes 8 seconds
max 58 minutes 26 seconds 53 minutes 0 seconds

Note: I changed the classPath method in Global to the dynamic dispatch. And now there's returned a base class of the old and the new classpath with common interface based on a part of API of the old classpath. I tested it with sbt and there's no problem with compiling projects etc. Previously Grzegorz created certain hack for sbt's compiler interface where current classpath was calling the flat one, when it was needed. I changed this ugly solution to this mentioned dynamic dispatch and removed a hack. We can return to something like this, if it'd turn out that it's necessary.

Thanks to Grzegorz for hints and his initial implementation.

Lastly, the below disclaimer is required by the lawyers:

THIS PROGRAM IS SUBJECT TO THE TERMS OF THE BSD 3-CLAUSE LICENSE.

THE FOLLOWING DISCLAIMER APPLIES TO ALL SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE:
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS “AS IS” AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA,
OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR
ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.
ONLY THE SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE, IF ANY, THAT ARE ATTACHED TO (OR OTHERWISE ACCOMPANY) THIS SUBMISSION (AND ORDINARY
COURSE CONTRIBUTIONS OF FUTURES PATCHES THERETO) ARE TO BE CONSIDERED A CONTRIBUTION. NO OTHER SOFTWARE CODE OR MATERIALS ARE A CONTRIBUTION.

mpociecha added some commits Nov 28, 2014

Abstract over ClassPath and ClassRep
This commit is intended to create the possibility to plug in into
the compiler an alternative classpath representation which would be
possibly more efficient, use less memory etc. Such an implementation
- at least at the beginning - should exist next to the currently
existing one and be possible to turn on using a flag.

Several places in the compiler have a direct dependency on the
classpath implementation. Examples include backend's icode generator
and reader, SymbolLoaders, ClassfileParser. After closer inspection,
one realizes that all those places depend only on a very small subset
of classpath logic: they need to lookup classes from classpath. Hence
there's introduced ClassFileLookup trait that encapsulates that
functionality. The ClassPath extends that trait and an alternative
one also must do it.

There's also added ClassRepresentation - the base trait for ClassRep
(the inner class of ClassPath). Thanks to that the compiler uses
a type which is not directly related to the particular classpath
representation as it was doing until now.
Use new asClassPathString method and create FileUtils for classpath
The method asClasspathString is now deprecated. Moreover it's moved
to ClassFileLookup in the case someone was using it in some project
(an alternative classpath also will support it - just in the case).
All its usages existing in Scala sources are changed to
asClassPathString method. The only difference is the name.

Some operations on files or their names are moved from ClassPath to
the newly created FileUtils dedicated to classpath. It will be
possible to reuse them when implementing an alternative classpath
representation. Moreover such allocation-free extension methods like
the one added in this commit will improve the readability.
Define interface for flat classpath and add package loader using it
This commit introduces the base trait for flat classpath - an
alternative classpath representation. In accordance with the idea
and the experimental implementation of @gkossakowski, this
representation will try to make the best use of the specificity of
a given file type instead of using AbstractFile everywhere. It's
possible as .NET backend is no longer supported and we can focus on
Java-specific types of files.

FlatClassPath extends ClassFileLookup which provides the common
interface used also by existing ClassPath.

The new implementation is called flat because it's possible to query
the whole classpath using just single instance.
In the case of the old (recursive) representation there's the
structure of nested classpath objects, where each such an object can
return only entries from one level of hierarchy but it returns also
another classpath objects for nested levels included in it.

That's why there's added dedicated PackageLoaderUsingFlatClassPath in
SymbolLoaders - approaches are different so also the way of loading
packages has to be different. The new package loader is currently
unused.

There's added also PackageNameUtils which will provide common methods
used by classpath implementations for various file types.
Add the flat classpath type aggregating flat classpath instances
There's added AggregateFlatClassPath - an equivalent of
MergedClassPath from the old implementation. It is supposed to group
classpath instances handling different files being directories, zips
or jars.

Unlike in the case of the old (recursive) implementation, there won't
be a deep, nested hierarchy of classpath instances - just one root
(aggregate) and a flat structure of its children.

AggregateFlatClassPath ensures the distinction of classpath entries
and merges corresponding entries for class and source files into one
entry. This is required as SymbolLoaders class makes use of this kind
of ClassRepresentation.

There are also added unit tests which check whether
AggregateFlatClassPath obtains correct entries from classpath
instances specified in a constructor and whether it preserves the
ordering in the case of repeated entries. There's added a test type
of flat classpath using VirtualFiles so it's easy to check the real
behaviour.
Add flat classpath implementation for directories
There's added the flat classpath implementation for directories using
java.util.File directly. Since we work with a real directory - not
the AbstractFile - we don't need to iterate all entries of a file to
get inner entries of some package. We can just find an adequate
directory for a package.

There are added implementations for a class- and a sourcepath. Both
extend DirectoryFileLookup which provides common logic.
Add flat classpath implementation for zip and jar files
This commit adds an implementation of flat classpath which can handle
both jar and vanilla zip files. In fact there are two versions - for
a class- and a sourcepath. Both extend ZipArchiveFileLookup which
provides common logic.

They use FileZipArchive. @gkossakowski made a comparison of different
ways of handling zips and jars (e.g. using javac's ZipFileIndex). He
stated that general efficiency of FileZipArchive, taking into account
various parameters, is the best.

FileZipArchive is slightly changed. From now it allows to find the
entry for directory in all directory entries without iterating all
entries regardless of a type. Thanks to that we can simply find
a directory for a package - like in the case of DirectoryFileLookup.

There's also added possibility to cache classpath representation of
classpath elements from jar and zip files across compiler instances.
The cache is just a map AbstractFile -> FlatClassPath. It should
reduce the number of created classpath and file instances e.g. in the
case of many ScalaPresentationCompilers in Scala IDE.

To prevent the possibility to avoid a cache, caches are created as
a part of factories responsible for the creation of these types of
the flat classpath.
Add flat classpath implementation using ManifestResources
There's added the flat classpath type using ManifestResources,
closely related to the support for JSR-223 (Scripting for the Java
Platform). It uses classes listed in the manifest file placed in the
JAR. It's related to jar files so it's created using
ZipAndJarFlatClassPathFactory and is cached.

In general currently it's not possible to use it in Scala out of the
box (without using additional tools such as jarlister) as this
support is postponed. The old classpath has been properly prepared in
the PR created by @rjolly #2238
so the new one also got this feature.

ManifestResources is a ZipArchive without a real underlying file
placed on a disk and in addition implementing some methods declared
in AbstractFile as unsupported operations. Therefore the
implementation has to use the iterator. I wanted to have the similar
behaviour as in the case of directories and zip/jar files - be able
to get a directory entry for a package without iterating all entries.
This is achieved by iterating all entries only once and caching
packages.

This flat classpath type was the last needed one.
Create base classpath factory and an implementation for the flat cp
The part of the functionality of a ClassPathContext has been moved
to the base trait ClassPathFactory so it can be reused by the newly
created FlatClassPathFactory. This new implementation works in
similar manner as the ClassPathContext with this difference that it
just creates instances of flat classpath.

This change doesn't modify the behaviour of the compiler as the
interface and the way ClassPathContext works didn't change. Moreover
FlatClassPathFactory is currently unused.
Create dedicated path resolver for the flat classpath representation
This commit adds dedicated FlatClassPathResolver loading classpath
entries as FlatClassPath.

Most of the common logic from PathResolver for the old classpath has
been moved to the base, separate class which isn't dependent on
a particular classpath representation. Thanks to that it was possible
to reuse it when creating an adequate path resolver for the flat
classpath representation.

This change doesn't modify the way the compiler works. It also
doesn't change nothing from the perspective of someone who already
uses PathResolver in some project or even extends it - at least as
long as he/she doesn't need to use flat classpath.

There are also added JUnit tests inter alia comparing entries created
using the old and the new classpath representations (whether the flat
one created using the new path resolver returns the same entries as
the recursive one).
@mpociecha

This comment has been minimized.

Show comment
Hide comment
@mpociecha

mpociecha Dec 1, 2014

Member

@gkossakowski @adriaanm I hope this time it's okay.

Member

mpociecha commented Dec 1, 2014

@gkossakowski @adriaanm I hope this time it's okay.

@scala-jenkins scala-jenkins added this to the 2.11.5 milestone Dec 1, 2014

Refactor scalap's main
The structure of scalap's Main has been refactored.

EmptyClasspath is deleted. It looks that it was unused since this
commit: e594fe5

Also classpath logging is changed and now uses asClassPathString
method. It was needed to modify one test because of that but it
won't depend on a particular representation.

There aren't changes in the way scalap works.
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Dec 1, 2014

Member

PLS REBUILD/pr-scala@7935085a583c8497a7153996113ca27875ad1c38

Member

adriaanm commented Dec 1, 2014

PLS REBUILD/pr-scala@7935085a583c8497a7153996113ca27875ad1c38

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Dec 1, 2014

Member

This looks really, really good! Thanks for going all the way, @mpociecha!

Member

adriaanm commented Dec 1, 2014

This looks really, really good! Thanks for going all the way, @mpociecha!

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Dec 1, 2014

(kitty-note-to-self: ignore 65023355)
🐱 Roger! Rebuilding pr-scala for 7935085. 🚨

scala-jenkins commented Dec 1, 2014

(kitty-note-to-self: ignore 65023355)
🐱 Roger! Rebuilding pr-scala for 7935085. 🚨

@gkossakowski

This comment has been minimized.

Show comment
Hide comment
@gkossakowski

gkossakowski Dec 1, 2014

Would marking ZipAndJarFileLookupFactory as sealed help to assure that this is the only way of creating flat classpath representations?

Would marking ZipAndJarFileLookupFactory as sealed help to assure that this is the only way of creating flat classpath representations?

This comment has been minimized.

Show comment
Hide comment
@mpociecha

mpociecha Dec 2, 2014

Owner

Certainly such a change won't do any harm.

Owner

mpociecha replied Dec 2, 2014

Certainly such a change won't do any harm.

@gkossakowski

This comment has been minimized.

Show comment
Hide comment
@gkossakowski

gkossakowski Dec 1, 2014

Member

I looked at the 7448377..672c119 range and it looked really good! With vastly improved commit messages and split of changes into separate, small commits it's really easy to review the code.

I'll continue the review of the rest of commits tomorrow.

Member

gkossakowski commented Dec 1, 2014

I looked at the 7448377..672c119 range and it looked really good! With vastly improved commit messages and split of changes into separate, small commits it's really easy to review the code.

I'll continue the review of the rest of commits tomorrow.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Dec 3, 2014

Member

@mpociecha, sorry about the radio silence -- my understanding is that we'll most likely be able to merge this as-is in the next few days.

Member

adriaanm commented Dec 3, 2014

@mpociecha, sorry about the radio silence -- my understanding is that we'll most likely be able to merge this as-is in the next few days.

@gkossakowski

This comment has been minimized.

Show comment
Hide comment
@gkossakowski

gkossakowski Dec 4, 2014

Member

For documentation purposes: previous iterations of this PR are #4060 and #3962.

Member

gkossakowski commented Dec 4, 2014

For documentation purposes: previous iterations of this PR are #4060 and #3962.

@gkossakowski

This comment has been minimized.

Show comment
Hide comment
@gkossakowski

gkossakowski Dec 4, 2014

Member

I just finished reviewing all commits. I'm really impressed with quality of this PR. Really good work on both documentation (in commit messages and in the code) and the code itself! Also the structure of commits is really good.

Here're the things that hold me from giving LGTM and merging:

  • determine status of "Copyright (c) 2014 Contributor. All rights reserved." headers added to the contributed code. I understand they are required by lawyers but we have to make sure we can merge code like this. Scala is a bit special because it has its own license and CLA. I'll defer this decision to @adriaanm
  • turn off flat classpath by default; I understand that it was enabled so we can test it on CI and that was a good move; however, we have to turn it off, let's do that in separate commit appended to at the end of the list of commits

I just kicked community build that will use scala compiler version submitted by this PR: https://jenkins-dbuild.typesafe.com:8499/job/Community-2.11.x-manual/102/console

However, even if we find some issues with it we can fix them in subsequent PRs. For now only the two tasks I outlined above are needed to be done for merging this PR.

Member

gkossakowski commented Dec 4, 2014

I just finished reviewing all commits. I'm really impressed with quality of this PR. Really good work on both documentation (in commit messages and in the code) and the code itself! Also the structure of commits is really good.

Here're the things that hold me from giving LGTM and merging:

  • determine status of "Copyright (c) 2014 Contributor. All rights reserved." headers added to the contributed code. I understand they are required by lawyers but we have to make sure we can merge code like this. Scala is a bit special because it has its own license and CLA. I'll defer this decision to @adriaanm
  • turn off flat classpath by default; I understand that it was enabled so we can test it on CI and that was a good move; however, we have to turn it off, let's do that in separate commit appended to at the end of the list of commits

I just kicked community build that will use scala compiler version submitted by this PR: https://jenkins-dbuild.typesafe.com:8499/job/Community-2.11.x-manual/102/console

However, even if we find some issues with it we can fix them in subsequent PRs. For now only the two tasks I outlined above are needed to be done for merging this PR.

@mpociecha

This comment has been minimized.

Show comment
Hide comment
@mpociecha

mpociecha Dec 4, 2014

Member
  1. I'm not sure but probably you can just change them after merging this PR. ;)
  2. Okay. It's exactly like my last sentences in the last commit:
    "Also flat classpath implementation is set as a default one to test it
    on Jenkins. This particular change must be reverted when all tests
    will pass because for now it's not desirable to make it permanently
    the default representation."

I'll read your questions/remarks and answer to them soon.

Member

mpociecha commented Dec 4, 2014

  1. I'm not sure but probably you can just change them after merging this PR. ;)
  2. Okay. It's exactly like my last sentences in the last commit:
    "Also flat classpath implementation is set as a default one to test it
    on Jenkins. This particular change must be reverted when all tests
    will pass because for now it's not desirable to make it permanently
    the default representation."

I'll read your questions/remarks and answer to them soon.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Dec 4, 2014

Member

I double checked and the copyright header is fine.

Member

adriaanm commented Dec 4, 2014

I double checked and the copyright header is fine.

mpociecha added some commits Nov 30, 2014

Integrate flat classpath with the compiler
This commit integrates with the compiler the whole flat classpath
representation build next to the recursive one as an alternative.

From now flat classpath really works and can be turned on. There's
added flag -YclasspathImpl with two options: recursive (the default
one) and flat.

It was needed to make the dynamic dispatch to the particular
classpath representation according to the chosen type of a classpath
representation.

There's added PathResolverFactory which is used instead of a concrete
implementation of a path resolver. It turned out that only a small
subset of path resolvers methods is used outside this class in Scala
sources. Therefore, PathResolverFactory returns an instance of a base
interface PathResolverResult providing only these used methods.

PathResolverFactory in combination with matches in some other places
ensures that in all places using classpath we create/get the proper
representation.

Also the classPath method in Global is modified to use the dynamic
dispatch. This is very important change as a return type changed to
the base ClassFileLookup providing subset of old ClassPath public
methods. It can be problematic if someone was using in his project
the explicit ClassPath type or public methods which are not provided
via ClassFileLookup. I tested flat classpath with sbt and Scala IDE
and there were no problems. Also was looking at sources of some other
projects like e.g. Scala plugin for IntelliJ and there shouldn't be
problems, I think, but it would be better to check these changes
using the community build.

Scalap's Main.scala is changed to be able to use both implementations
and also to use flags related to the classpath implementation.

The classpath invalidation is modified to work properly with the old
(recursive) classpath representation after changes made in a Global.
In the case of the attempt to use the invalidation for the flat cp it
just throws exception with a message that the flat one currently
doesn't support the invalidation. And also that's why the partest's
test for the invalidation has been changed to use (always) the old
implementation. There's added an adequate comment with TODO to this
file.

There's added partest test generating various dependencies
(directories, zips and jars with sources and class files) and testing
whether the compilation and further running an application works
correctly, when there are these various types of entries specified as
-classpath and -sourcepath. It should be a good approximation of real
use cases.
Create possibility to skip flat classpath caching
There's added -YdisableFlatCpCaching option to ScalaSettings which
allows user to disable caching of flat representation of classpath
elements.
Cleanup and refactoring - semicolons, unused or commented out code
This commit contains some minor changes made by the way when
implementing flat classpath.

Sample JUnit test that shows that all pieces of JUnit infrastructure
work correctly now uses assert method form JUnit as it should do from
the beginning.

I removed commented out lines which were obvious to me. In the case
of less obvious commented out lines I added TODOs as someone should
look at such places some day and clean them up.

I removed also some unnecessary semicolons and unused imports.

Many string concatenations using + have been changed to string
interpolation.

There's removed unused, private walkIterator method from ZipArchive.
It seems that it was unused since this commit:
9d4994b
However, I had to add an exception for the compatibility checker
because it was complaining about this change.

I made some trivial corrections/optimisations like use 'findClassFile'
method instead of 'findClass' in combination with 'binary' to find
the class file.
Add benchmarks to compare recursive and flat cp representations
The goal of these changes is to add possibility to:
- compare an efficiency and a content of both cp implementations
(ClassPathImplComparator)
- examine the memory consumption by creating a lot of globals using
a specified classpath (ClassPathMemoryConsumptionTester) - it can be
considered as e.g. some approximation of ScalaPresentationCompilers
in Scala IDE when working with many projects

ClassPathMemoryConsumptionTester is placed in main (I mean not test)
sources so thanks to that it has properly, out of the box configured
boot classpath etc. and it's easy to use it, e.g.:

scala scala.tools.nsc.ClassPathMemoryConsumptionTester
-YclasspathImpl:<implementation_to_test> -cp <some_cp>
-sourcepath <some_sp> -requiredInstances 50 SomeFileToCompile.scala

At the end it waits for the "exit" command so there can be used some
profiler like JProfiler to look how the given implementation behaves.

Also flat classpath implementation is set as a default one to test it
on Jenkins. This particular change must be reverted when all tests
will pass because for now it's not desirable to make it permanently
the default representation.
Turn off flat classpath by default, mark one of its classes as sealed
This commit addresses code review comments.

The flat classpath is no longer the default classpath representation.
It was the default one just for the test purposes. For now it's not
desirable to make it permanently the default representation.

ZipAndJarFileLookupFactory is marked as sealed - it should help to
limit the ways of creating flat classpath instances for zips and jars.

@scala-jenkins scala-jenkins removed the tested label Dec 5, 2014

@mpociecha

This comment has been minimized.

Show comment
Hide comment
@mpociecha

mpociecha Dec 5, 2014

Member

@gkossakowski Error messages and a value name in t6502.scala are corrected directly in "Integrate flat classpath with the compiler". The last commit fixes everything else.

Member

mpociecha commented Dec 5, 2014

@gkossakowski Error messages and a value name in t6502.scala are corrected directly in "Integrate flat classpath with the compiler". The last commit fixes everything else.

@scala-jenkins scala-jenkins added the tested label Dec 5, 2014

@gkossakowski

This comment has been minimized.

Show comment
Hide comment
@gkossakowski

gkossakowski Dec 5, 2014

Member

LGTM

The whole community build passed with flat classpath enabled. Excellent work! 👏 👏 👏

Member

gkossakowski commented Dec 5, 2014

LGTM

The whole community build passed with flat classpath enabled. Excellent work! 👏 👏 👏

gkossakowski added a commit that referenced this pull request Dec 5, 2014

Merge pull request #4176 from mpociecha/flat-classpath2
The alternative, flat representation of classpath elements

@gkossakowski gkossakowski merged commit 124cf2f into scala:2.11.x Dec 5, 2014

1 check passed

default pr-scala Took 111 min.
Details
@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Dec 5, 2014

Member

nice! 💥

Member

lrytz commented Dec 5, 2014

nice! 💥

@mpociecha

This comment has been minimized.

Show comment
Hide comment
@mpociecha

mpociecha Dec 5, 2014

Member

Thank you all for your help and patience.

Member

mpociecha commented Dec 5, 2014

Thank you all for your help and patience.

@gkossakowski

This comment has been minimized.

Show comment
Hide comment
@gkossakowski

gkossakowski Dec 9, 2014

Member

@mpociecha, since this PR receives a fair amount of traffic (3500 views already) I edited the original description to provide a very high level info first. In particular, I mentioned how to test it and in which scenarios it makes sense to do it.

Member

gkossakowski commented Dec 9, 2014

@mpociecha, since this PR receives a fair amount of traffic (3500 views already) I edited the original description to provide a very high level info first. In particular, I mentioned how to test it and in which scenarios it makes sense to do it.

@mpociecha

This comment has been minimized.

Show comment
Hide comment
@mpociecha

mpociecha Dec 9, 2014

Member

Ok, thanks. I didn't know there are such statistics. :)

Member

mpociecha commented Dec 9, 2014

Ok, thanks. I didn't know there are such statistics. :)

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Dec 10, 2014

Member

parallel builds in sbt

@gkossakowski I remember we were going to validate that the SBT setup would profit from this setting. Was this done?

Member

retronym commented Dec 10, 2014

parallel builds in sbt

@gkossakowski I remember we were going to validate that the SBT setup would profit from this setting. Was this done?

@gkossakowski

This comment has been minimized.

Show comment
Hide comment
@gkossakowski

gkossakowski Dec 10, 2014

Member

It wasn't until now. I created sbt project with the following config:

resolvers in Global += Resolver.sonatypeRepo("snapshots")
scalaVersion in Global := "2.11.5-SNAPSHOT"
//scalacOptions in Global += "-YclasspathImpl:flat"
// allow 20 different compiler instances to run in parallel
concurrentRestrictions in Global := Tags.limitAll(20) :: Nil
lazy val project1 = project
lazy val project2 = project
...
lazy val project19 = project
lazy val project20 = project

Each project has one source file and no extra dependencies apart from standard Java and Scala dependencies.

I attached Yourkit profiler, ran compile and observed memory consumption. After that, I enabled flat classpath, forced gc and rerun compile again. The chart I got is:

image

If you measure memory consumption caused just by compilation (by subtracting the memory consumed by sbt itself) you get:

Classpath impl Memory consumed
recursive (old) 440 MB
flat (new) 264 MB

The difference in memory consumption is not as dramatic as reported originally in the PR but still very noticeable. The gain we get depends on classpath's size. Standard classpath is small.

Member

gkossakowski commented Dec 10, 2014

It wasn't until now. I created sbt project with the following config:

resolvers in Global += Resolver.sonatypeRepo("snapshots")
scalaVersion in Global := "2.11.5-SNAPSHOT"
//scalacOptions in Global += "-YclasspathImpl:flat"
// allow 20 different compiler instances to run in parallel
concurrentRestrictions in Global := Tags.limitAll(20) :: Nil
lazy val project1 = project
lazy val project2 = project
...
lazy val project19 = project
lazy val project20 = project

Each project has one source file and no extra dependencies apart from standard Java and Scala dependencies.

I attached Yourkit profiler, ran compile and observed memory consumption. After that, I enabled flat classpath, forced gc and rerun compile again. The chart I got is:

image

If you measure memory consumption caused just by compilation (by subtracting the memory consumed by sbt itself) you get:

Classpath impl Memory consumed
recursive (old) 440 MB
flat (new) 264 MB

The difference in memory consumption is not as dramatic as reported originally in the PR but still very noticeable. The gain we get depends on classpath's size. Standard classpath is small.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Dec 15, 2014

Member

Thanks for verifying this.

Member

retronym commented Dec 15, 2014

Thanks for verifying this.

@som-snytt

This comment has been minimized.

Show comment
Hide comment
@som-snytt

som-snytt Feb 9, 2015

Contributor

The parens are the paulpism for saying, This is a single expr. It avoids mistakes due to the following line.

Contributor

som-snytt commented on src/compiler/scala/tools/util/PathResolver.scala in a8c43dc Feb 9, 2015

The parens are the paulpism for saying, This is a single expr. It avoids mistakes due to the following line.

@som-snytt

This comment has been minimized.

Show comment
Hide comment
@som-snytt

som-snytt Feb 9, 2015

Contributor

For embedded newlines, it's preferable to use f"foo%nbar" or import scala.compat.Platform.EOL ; s"foo${EOL}bar". On Windows, you wind up with ugly mixed line endings that confuse vim.

Contributor

som-snytt commented on src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala in a8c43dc Feb 9, 2015

For embedded newlines, it's preferable to use f"foo%nbar" or import scala.compat.Platform.EOL ; s"foo${EOL}bar". On Windows, you wind up with ugly mixed line endings that confuse vim.

This comment has been minimized.

Show comment
Hide comment
@mpociecha

mpociecha Feb 9, 2015

Member

Ok. Could you correct this in one of your next PRs? I assume you'll probably create some in the nearest future.

Member

mpociecha replied Feb 9, 2015

Ok. Could you correct this in one of your next PRs? I assume you'll probably create some in the nearest future.

This comment has been minimized.

Show comment
Hide comment
@som-snytt

som-snytt Feb 9, 2015

Contributor

Yes, I was just evangelizing. Rebasing an old commit had a conflict with this one, is why I'm here...

Contributor

som-snytt replied Feb 9, 2015

Yes, I was just evangelizing. Rebasing an old commit had a conflict with this one, is why I'm here...

@som-snytt

This comment has been minimized.

Show comment
Hide comment
@som-snytt

som-snytt Jul 11, 2015

Contributor

This means if I use the method, my code won't run on earlier point releases, which is Mima's whole point. Kind of surprised this got by the censors, since it's trivial to implement as an extension method. It might have been preferable to add it, if necessary, on the nsc side.

Contributor

som-snytt commented on bincompat-forward.whitelist.conf in a8c43dc Jul 11, 2015

This means if I use the method, my code won't run on earlier point releases, which is Mima's whole point. Kind of surprised this got by the censors, since it's trivial to implement as an extension method. It might have been preferable to add it, if necessary, on the nsc side.

@retronym retronym referenced this pull request Aug 17, 2017

Open

Improve Classpath Implementation #416

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment