scala3 icon indicating copy to clipboard operation
scala3 copied to clipboard

Port JVM backend refactor from Scala 2

Open WojciechMazur opened this issue 2 years ago • 7 comments

This PR ports JVM backend refactor from Scala 2 as part of the https://github.com/lampepfl/dotty/issues/14912 thread. It squashes changes based on the PRs:

  • [x] https://github.com/scala/scala/pull/6012
  • [x] https://github.com/scala/scala/pull/6057

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

WojciechMazur avatar May 30 '22 09:05 WojciechMazur

test performance please

WojciechMazur avatar Jun 01 '22 08:06 WojciechMazur

test performance please

bishabosha avatar Jun 01 '22 09:06 bishabosha

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

dottybot avatar Jun 01 '22 09:06 dottybot

Performance test finished successfully:

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

Benchmarks is based on merging with main (8d13cbbf53)

dottybot avatar Jun 01 '22 11:06 dottybot

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.

sjrd avatar Jul 20 '22 09:07 sjrd

fyi @lrytz

SethTisue avatar Jul 20 '22 17:07 SethTisue

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

WojciechMazur avatar Aug 01 '22 15:08 WojciechMazur

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.

TheElectronWill avatar Dec 01 '22 11:12 TheElectronWill

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

SethTisue avatar Jan 30 '23 21:01 SethTisue

@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.

WojciechMazur avatar Feb 01 '23 14:02 WojciechMazur

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

sjrd avatar Feb 01 '23 14:02 sjrd

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.

WojciechMazur avatar Mar 28 '23 08:03 WojciechMazur

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

🎉🎉🎉

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

SethTisue avatar Mar 28 '23 12:03 SethTisue