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

Port JVM backend refactor from Scala 2 #15322

Merged
merged 1 commit into from
Mar 28, 2023
Merged

Conversation

WojciechMazur
Copy link
Contributor

@WojciechMazur WojciechMazur commented May 30, 2022

This PR ports JVM backend refactor from Scala 2 as part of the #14912 thread.
It squashes changes based on the PRs:

The last refactor introducing backend parallelism would be introduced in the follow up PR

@WojciechMazur WojciechMazur force-pushed the port/scala-6012 branch 2 times, most recently from b51f668 to fc9b098 Compare May 31, 2022 22:57
@WojciechMazur WojciechMazur marked this pull request as ready for review June 1, 2022 08:43
@WojciechMazur
Copy link
Contributor Author

test performance please

1 similar comment
@bishabosha

This comment was marked as outdated.

@dottybot
Copy link
Member

dottybot commented Jun 1, 2022

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

@dottybot
Copy link
Member

dottybot commented Jun 1, 2022

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/15322/ to see the changes.

Benchmarks is based on merging with main (8d13cbb)

compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/jvm/BTypes.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/jvm/BackendUtils.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/jvm/ClassfileWriter.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/jvm/CoreBTypes.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/jvm/GenBCode.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/jvm/CodeGen.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/jvm/GenBCode.scala Outdated Show resolved Hide resolved
@sjrd
Copy link
Member

sjrd commented Jul 20, 2022

The commits "Don't create semanticdb files when writing to JAR" and "Use JarWriter only when target jar is empty" don't seem to belong to this PR. They don't correspond to anything in the mentioned scala/scala PRs.

The last commit about whitespace should be squashed with relevant previous commits.


Sorry for the very late review. :s It was a bit of a daunting one.

@SethTisue
Copy link
Member

fyi @lrytz

@WojciechMazur
Copy link
Contributor Author

The commits "Don't create semanticdb files when writing to JAR" and "Use JarWriter only when target jar is empty" don't seem to belong to this PR. They don't correspond to anything in the mentioned scala/scala PRs.

Yes, that's a thing to issues present only in Scala 3. During the refactoring I was present with some silent issues, producing empty jars or containing only a single file even though jar size suggested it has more data. It happened due to content malformation - in case if output directory (jar file in our case) was opened and written directly using FileOutputStreams we were able to correctly read it's content. SementicDB phase was writing some metadata to output in one of the early phases. However if we would open this file again at a later stage and entries we would write would be unable to read, probably due to repeated header inserted by JarWritter. I can move it separate PR, but since commits are not squashed we still would be able to track this single changes

@TheElectronWill
Copy link
Contributor

Could https://github.com/lampepfl/dotty/blob/main/docs/_docs/internals/backend.md be updated too? It seems that the schema at the top is outdated with respect to this refactoring.

WojciechMazur added a commit that referenced this pull request Jan 30, 2023
…fined as JAR. (#16790)

Change extracted from #15322 where
the issue was discovered

Writing to outputDirectory when it is a JAR can lead to producing
malformed file, it is using FileOutputStream which works somehow
correct, but if we would open the same JAR again in later phase (eg.
genBCode) header of the file would be malformed: the result file grows
in size respectively to provided input, but we can read only files
written by SemanticDB phase.
@SethTisue
Copy link
Member

happy to see this springing back to life — I'm curious if there's a specific motivation for reviving it, or if you just happen to be getting around to it

@WojciechMazur
Copy link
Contributor Author

@sjrd The mentioned changes were extracted and merged in a separate PR along with updated documentation. So if you'll have a bit of spare time it should be read for a new round of review.

@sjrd
Copy link
Member

sjrd commented Feb 1, 2023

I have put it at the top of my TODO list. I should have time for this tomorrow.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

At this point, the second commit is 99% fixups for the first commit, and only 1% of its changes actually correspond to its commit message.

At least squash them. Even better: still extract the second commit but in a way that its changes are really only about what it says it does. The corresponding commit in Scala 2 is tiny.

Otherwise, looks good.

@sjrd sjrd assigned WojciechMazur and unassigned sjrd Feb 2, 2023
@WojciechMazur
Copy link
Contributor Author

Even better: still extract the second commit but in a way that its changes are really only about what it says it does. The corresponding commit in Scala 2 is tiny.

The corresponding commit has no real value in the case of Scala 3 - there is no optimizer in the JVM backend, because of that I've decided to squash the changes together.

@sjrd sjrd merged commit 3948ecc into scala:main Mar 28, 2023
@SethTisue
Copy link
Member

It's a cold day in hell today!!!

🎉🎉🎉

Thank you so much @WojciechMazur and @sjrd for seeing this through!

@WojciechMazur WojciechMazur deleted the port/scala-6012 branch March 28, 2023 12:26
bishabosha added a commit that referenced this pull request Oct 10, 2023
This PR ports scala/scala#6124 introduces
backend parallelism, on top of the previously ported changes introduced
in #15322
Adds Scala2 flags: 
* `-Yjar-compression-level:N`
* `-Ybackend-parallelism:N`
* `-Ybackend-worker-queue:N`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants