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

Timestamp hashes #434

Closed
wants to merge 1 commit into from
Closed

Timestamp hashes #434

wants to merge 1 commit into from

Conversation

fommil
Copy link

@fommil fommil commented Oct 14, 2017

Because of sbt/sbt#363 I am unable to say anything about the perf characteristics of this PR.

I guess it is addressing the same problem that #371 is addressing and a hacky alternative to #433 (which is probably the correct solution).

What I thought was an I/O bottleneck in sbt/io#79 turned out to be a CPU hashcode bottleneck when hashing classpath files, not source files. YourKit doesn't pick up the hash/crypto work, showing it as InputFilterStream. Given the many many bugs in the entire scala tooling chain when it comes to .jar files mutating on disk, see https://gitlab.com/fommil/class-monkey/blob/master/README.md, I really don't think SHA-1 is needed here. Timestamps (and file size) should be more than enough.

Also, this hashing could be done in parallel. Even if it's as simple as using the terrible .par.

@lightbend-cla-validator

Hi @fommil,

Thank you for your contribution! We really value the time you've taken to put this together.

We see that you have signed the Lightbend Contributors License Agreement before, however, the CLA has changed since you last signed it.
Please review the new CLA and sign it before we proceed with reviewing this pull request:

http://www.lightbend.com/contribute/cla

@fommil fommil changed the base branch from 1.x to 1.0.x October 14, 2017 10:46
@fommil
Copy link
Author

fommil commented Oct 14, 2017

BTW, I'm not signing the Lightbend CLA... you can have my contributions under the terms of the BSD-3, the license of this project. Otherwise it is not a symmetric and fair agreement.

@fommil
Copy link
Author

fommil commented Oct 14, 2017

I'd like to do some more work on this, but I'm basically blocked until I can try out my changes locally without having to build the entire sbt ecosystem from scratch... I just want to be able to stick a jar at the right place in the sbt classpath.

@fommil
Copy link
Author

fommil commented Oct 14, 2017

closing in preference of #433

@fommil fommil closed this Oct 14, 2017
@fommil fommil deleted the timestamp-hashes branch October 14, 2017 12:14
@romanowski
Copy link
Contributor

@fommil We also have problems with classpath hashing (you can probably imagine) and caching was best approach for us.
I've open PR ( #427) that allows client o implement classpath hashing on thier side (and I think it should be possible to override this from sbt level).

@fommil
Copy link
Author

fommil commented Oct 14, 2017

Typical @romanowski : "yeah, we already seen this and fixed it months ago... 🤷‍♂️ " 😁

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