-
-
Notifications
You must be signed in to change notification settings - Fork 369
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
Use classpath PathRef
s hashCode as cache key for Scala.js worker
#2183
Conversation
026e883
to
77ac0f9
Compare
private def bridge(toolsClasspath: Agg[mill.PathRef])(implicit ctx: Ctx.Home) = { | ||
// toolsClasspath is stable and made of external libraries which have the versions | ||
// in the file names. So we use the paths string to hash since it's ~50x faster | ||
val classloaderSig = toolsClasspath.iterator.map(p => p.path.hashCode).sum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using the hashCode
of the PathRef
is a better option. Using only the name is not guaranteed to be stable. E.g. you could dynamically download to a same local path T.dest / "out.jar"
and would miss any change when this jar changes later.
This makes the `bridge` method take 15ms instead of 600ms It is safe since the filenames contain the Scala.js version, so if you switch version the hashCode will be different as expected
77ac0f9
to
8a32ac7
Compare
I naively thought that calculating the hashcode of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. After you update the PR title and description, I think we are good to go.
PathRef
s hashCode as cache key for Scala.js worker
@lefou Done! Thank you for the review! I'm so happy about these performance improvements. Mill is noticeably faster! |
Do you have any numbers? EDIT: Oh, you already provided them in the description. Yeah, nice speed up! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change! Less code, better performance.
…om-lihaoyi#2183) This makes the `bridge` method take 15ms instead of 600ms (in my machine) since it is not needed anymore to perform file system operations. But we use the already existing `Agg[PathRef]` as the key to cache Scala.js workers. Pull request: com-lihaoyi#2183
This makes the
bridge
method take 15ms instead of 600ms (in my machine) since it is not needed anymore to perform file system operations. But we use the already existingAgg[PathRef]
as the key to cache Scala.js workers.