Skip to content

improvement: Change the stamps to timewrapped ones #1871

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

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Nov 24, 2022

Previously we would use last modfied stamps for classfiles (and other products) and hashes for jars. Now, we use hashes for both, but only if the modified time changed.

Should help with scalameta/metals#4623

@tgodzik tgodzik force-pushed the time-wrap-stamps branch 2 times, most recently from 85c9962 to 3fd64a7 Compare November 24, 2022 11:19
Previously we would use last modfied stamps for classfiles (aand other products) and hashes for jars. Now, we use hashes for both, but only if the modified time changed.

Should help with scalameta/metals#4623
@tgodzik tgodzik requested review from dos65 and Duhemm November 24, 2022 12:54
@tgodzik tgodzik marked this pull request as ready for review November 24, 2022 12:54
@tgodzik
Copy link
Contributor Author

tgodzik commented Nov 24, 2022

Locally run benchmark seems to produce the same timings and I am running the server one to confirm.

Copy link
Collaborator

@Duhemm Duhemm left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

I'll try that out and let you know how that may affect the performance on larger builds. Don't wait for me though 😄

fromBloopHashToZincHash(ByteHasher.hashFileContents(converter.toPath(file).toFile()))
def forHash(fileRef: VirtualFileRef): Hash = {
val file = converter.toPath(fileRef).toFile()
if (file.exists())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to check if the file exists now but not before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only used that for libraries, so they usually never got deleted, but if they did we would throw an exception. I now use it for classfiles, so they might be deleted more often and it's expected.

@bloopoid
Copy link
Collaborator

Performance test finished successfully.

Benchmarks is based on merging with master

2 similar comments
@bloopoid
Copy link
Collaborator

Performance test finished successfully.

Benchmarks is based on merging with master

@bloopoid
Copy link
Collaborator

Performance test finished successfully.

Benchmarks is based on merging with master

@tgodzik tgodzik merged commit c910a45 into scalacenter:main Nov 25, 2022
@tgodzik tgodzik deleted the time-wrap-stamps branch November 25, 2022 18:41
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.

None yet

3 participants