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

Regression: fix compilation performance on Windows #20193

Merged
merged 3 commits into from
May 7, 2024
Merged

Conversation

OlegYch
Copy link
Contributor

@OlegYch OlegYch commented Apr 15, 2024

Caches isDirectory calls

Too many of them were added in 607e4d5 and this degraded compilation performance by up to 100% on Windows

Fixes #19924
backport to release-3.4.2

@OlegYch
Copy link
Contributor Author

OlegYch commented Apr 15, 2024

signed CLA

@som-snytt
Copy link
Contributor

We need a "platform linter".

compiler/src/dotty/tools/io/PlainFile.scala Outdated Show resolved Hide resolved
Copy link
Contributor

@jchyb jchyb left a comment

Choose a reason for hiding this comment

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

Removing create() and delete() directories is dangerous - dotty.tools.io is a stable library that is being used in other projects, and those methods were implemented for PlainDirectory and PlainFile. With that said I looked through GitHub for all of the usages of those two, and they do not appear to be used anywhere (e.g. PlainDirectory is used in twirl and scalafix, but only to pass output directory to the compiler, PlainFile is only used in the compiler). With that in mind, and considering the performance benefit, I believe this should be merged

@jchyb jchyb dismissed nicolasstucki’s stale review May 7, 2024 16:38

Nicolas is unavailable, however all of the problems raised during the review were resolved by the PR creator

@jchyb jchyb merged commit 17c8766 into scala:main May 7, 2024
19 checks passed
@jchyb
Copy link
Contributor

jchyb commented May 7, 2024

Looks like in between the reviews I embarrassingly added a PlainFile.delete() method (which I of course forgot about), which of course I missed during my check right now, meaning we have a failing main. I will revert the revert after removing that delete method for the next release. Apologies for the inconvenience!

jchyb added a commit that referenced this pull request May 7, 2024
…#20355)

Reverts #20193
Cartoonishly failed right after I checked every other project for the
existence of `create` and `delete` methods... except the compiler
itself, where I personally added those a few weeks ago.
I will remove those for the next release, so we can re-merge this
performance PR.
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
jchyb added a commit that referenced this pull request May 17, 2024
Previously reverted due to conflicts on main just before 3.5.0 cutoff. 
Original PR by @OlegYch (#20193). I
added an additional commit fixing the issues (like in #20358)
@OlegYch
Copy link
Contributor Author

OlegYch commented Jul 30, 2024

is it possible to add this to 3.4.3 ?

@SethTisue
Copy link
Member

SethTisue commented Aug 7, 2024

is it possible to add this to 3.4.3 ?

(I don't make these decisions myself but) that seems implausible, as 3.4.3 wasn't even a planned release, it's a special hotfix release for a particular problem affecting compatibility, as per https://contributors.scala-lang.org/t/3-4-3-release-thread/6718

for some time now project attention has been on the 3.3.x and 3.5.x, and forthcoming 3.6.x series

also 3.5.0 is imminent, I hope you will be able to upgrade to it

@OlegYch
Copy link
Contributor Author

OlegYch commented Aug 26, 2024

@SethTisue i don't think this ended up in 3.5.0
but it seems to be present in 3.5.1-RC2
just a heads up

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.

Compilation performance regression on 3.4.0
7 participants