Skip to content
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

Default to emitting .tasty files (not .hasTasty) #4467

Merged
merged 7 commits into from
Jul 19, 2018

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented May 5, 2018

TODO

  • Default to emitting .tasty files
  • Fix filesystem bug when reading tasty in jars (partally?)

@nicolasstucki nicolasstucki self-assigned this May 5, 2018
@Blaisorblade Blaisorblade changed the title Always emit .tasty files (not .hasTasty) Default to emitting .tasty files (not .hasTasty) May 5, 2018
@nicolasstucki nicolasstucki reopened this May 22, 2018
@@ -404,8 +404,11 @@ trait ParallelTesting extends RunnerOrchestration { self =>
tastyOutput.mkdir()
val flags = flags0 and ("-d", tastyOutput.getAbsolutePath) and "-from-tasty"

def hasTastyFileToClassName(f: JFile): String =
targetDir.toPath.relativize(f.toPath).toString.dropRight(".hasTasty".length).replace('/', '.')
def hasTastyFileToClassName(f: JFile): String = {
Copy link
Contributor

Choose a reason for hiding this comment

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

name is now misleading

def hasTastyFileToClassName(f: JFile): String = {
val pathStr = targetDir.toPath.relativize(f.toPath).toString.replace('/', '.')
if (pathStr.endsWith(".hasTasty")) pathStr.dropRight(".hasTasty".length)
else pathStr.dropRight(".tasty".length)
Copy link
Contributor

Choose a reason for hiding this comment

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

.stripSuffix(".hasTasty").stripSuffix(".hasTasty")?

@nicolasstucki nicolasstucki force-pushed the always-emit-tasty branch 5 times, most recently from 94c8c88 to 5de4a9d Compare May 24, 2018 06:48
@@ -700,7 +700,7 @@ object PatternMatcher {
}
val newArgs =
for {
(rhs, actual) <- seenVars.toList
(rhs, actual) <- seenVars.toList.sortBy(_._2.name.toString)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have an ordering on Name (i.e. Names.NameOrdering), you shouldn't need to do toString

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to test if this is the source of the idempotency failure.

@nicolasstucki nicolasstucki force-pushed the always-emit-tasty branch 6 times, most recently from 8de6223 to ab6d5fa Compare May 24, 2018 15:34
@nicolasstucki nicolasstucki force-pushed the always-emit-tasty branch 3 times, most recently from 3a3cc05 to 930cff2 Compare June 18, 2018 16:44
@nicolasstucki nicolasstucki force-pushed the always-emit-tasty branch 5 times, most recently from f7fb52d to 308b1fb Compare June 21, 2018 09:58
@nicolasstucki nicolasstucki removed their assignment Jun 21, 2018
@dottybot
Copy link
Member

performance test scheduled: 30 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4467/ to see the changes.

Benchmarks is based on merging with master (bdf3ef4)

@nicolasstucki
Copy link
Contributor Author

Rebased

@nicolasstucki
Copy link
Contributor Author

@allanrenucci could you review the last commit

val path = classfile.path.stripSuffix(".class") + ".tasty"
val stream = cl.getResourceAsStream(path)
if (stream != null) {
val tasty = new Array[Byte](stream.available())
Copy link
Contributor

Choose a reason for hiding this comment

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

From the Javadoc

Note that while some implementations of InputStream will return the total number of bytes in the stream, many will not. It is never correct to use the return value of this method to allocate a buffer intended to hold all data in this stream.

val jarFile = JarArchive.open(io.File(jar.jpath))
try readTastyForClass(jarFile.jpath.resolve(classfile.path))
finally jarFile.close()
val cl = new URLClassLoader(Array(jar.jpath.toUri.toURL))
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure we won't have to deal with the same issues using a URLClassLoader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say no because the classloader never instanciates an class with a reference to it. Hence the classloader should be GCed at some point. I will experiment a bit with the profiler to see if I can observe any leaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not only leaks. Can the jar be read using an URLClassLoader while at the same time it is read using nio.file.FileSystems. What if someone close the FileSystems while you're reading the jar with URLClassLoader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it and it does not affect the class loader. I closed the file system in the middle of reading from the stream and some other places.

if (stream != null) {
val tasty = Array.newBuilder[Byte]
tasty.sizeHint(stream.available())
while (stream.available() > 0) // TODO improve performance
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the proper way to read from an InputStream, is to read() until -1 is returned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it

@nicolasstucki
Copy link
Contributor Author

Rebased

@odersky
Copy link
Contributor

odersky commented Jul 19, 2018

Ready to merge?

@nicolasstucki
Copy link
Contributor Author

nicolasstucki commented Jul 19, 2018

It is ready to merge. But after it is merged everyone needs to clean their out/ directory (to run sbt dotty-bootstrapped/...).

@nicolasstucki
Copy link
Contributor Author

I ran the sbt-dotty/scripted tests locally and source-dependencies / continuations failed. Will test it on the CI

@nicolasstucki
Copy link
Contributor Author

sbt-dotty/scripted passed on the CI http://dotty-ci.epfl.ch/lampepfl/dotty/6302

@nicolasstucki nicolasstucki merged commit fe8d050 into scala:master Jul 19, 2018
@allanrenucci allanrenucci deleted the always-emit-tasty branch July 19, 2018 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants