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

Bring back windows performance improvements #20423

Merged
merged 2 commits into from
May 17, 2024

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented May 16, 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)

jchyb added 2 commits May 16, 2024 22:33
This reverts commit c565993, which was
sound, but originally reverted due to merge conflicts on main.
@@ -78,7 +77,7 @@ class PlainFile(val givenPath: Path) extends AbstractFile {
}

/** Is this abstract file a directory? */
def isDirectory: Boolean = givenPath.isDirectory
val isDirectory: Boolean = givenPath.isDirectory // cached for performance on Windows
Copy link
Member

@bishabosha bishabosha May 17, 2024

Choose a reason for hiding this comment

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

What I assume is that this was a def because create() and delete() exist - but if they were never used does that mean in all code paths we never create/delete a directory before (but after object construction)/after testing isDirectory? (e.g. with java.nio api) - because then that might change behaviour especially if the object is cached somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in the original PR that was a concern, as this was a def because of the create and delete methods, but needed to be changed in the PR to a val for performance. As you mention this conflicted with the create() and delete() methods, but they turned out to not be used anywhere, so they were removed. From what I see in dotty PlainFiles and PlainDirectories always use java File api for creating and deleting the files and VirtualFiles and VirtualDirectories use internal representation, so they end up not needing the create and delete methods (because for that we call new or just dereference them from other VirtualDirectories). The exception here was the (now removed) call to PlainFile.delete() added after the CI pass of the original PR, which caused the conflicts on main

@jchyb jchyb merged commit 06ffc8c into scala:main May 17, 2024
19 checks passed
@jchyb jchyb deleted the bring-back-windows-improvements branch May 17, 2024 15:51
@Kordyjan Kordyjan added this to the 3.5.1 milestone Jul 3, 2024
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.

3 participants