-
Notifications
You must be signed in to change notification settings - Fork 6
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
Speed up ZincRunner #76
Conversation
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.
Thanks for doing this. I took a quick look through and left some minor comments.
workRequest.compilerOptions ++ | ||
workRequest.compilerOptionsReferencingPaths | ||
).toArray, | ||
workRequest.plugins.map(p => s"-Xplugin:$p").toArray ++ |
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.
Is there any benefit in doing workRequest.plugins.view.map(p => s"-Xplugin:$p").toArray
to avoid the construction of intermediate results?
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.
If so, I kind of wonder if there would be a benefit to using views in other places as well. I doubt it would be a huge win, but probably something.
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.
Yup, you're right; that would be more performant. I wasn't able to find any places in CommonArguments.scala
where .view
could be effectively utilized, but I found a few other places in ZincRunner
. I'll update them.
@@ -319,7 +312,17 @@ object ZincRunner extends WorkerMain[ZincRunnerWorkerConfig] { | |||
infos = FilteredInfos.getFilteredInfos(originalResultAnalysis.infos), | |||
) | |||
} | |||
analysisStoreText.set(AnalysisContents.create(resultAnalysis, compileResult.setup)) | |||
|
|||
if (verbosity >= 10) { |
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 recommend adding a comment here that says verbosity = 10 when --worker_verbose
is set. Otherwise this may look like an arbitrary constant to folks who don't know that.
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.
Good idea.
.withSources(sources.map(source => PlainVirtualFile(source.toAbsolutePath().normalize())).toArray) | ||
.withClasspath((classesOutputDir +: deps.map(_.classpath)).map(path => PlainVirtualFile(path)).toArray) | ||
.withSources(sources.view.map(source => PlainVirtualFile(source.toAbsolutePath().normalize())).toArray) | ||
.withClasspath((classesOutputDir +: deps.map(_.classpath)).view.map(path => PlainVirtualFile(path)).toArray) |
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.
Should deps.map(_.classpath)).view.map(path => PlainVirtualFile(path)).toArray)
instead be deps.view.map(_.classpath)).map(path => PlainVirtualFile(path)).toArray)
?
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.
Ah, good point. I didn't realize you could prepend to a View
.
32f6fdc
to
ba4c428
Compare
Rebased off of |
625ca3c
to
4f8c6d1
Compare
Squashed. |
These changes cause compilation to take ~98% of
ZincRunner
's time, up from ~93%—a 5% speedup.Before:
After: