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
Make compiling decisions internal #2942
Conversation
fd6bc70
to
59dbfc5
Compare
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.
Looks good, it should simplify life of native plugin maintainers
Thanks, I just need to make a few more naming and small changes before I take it out of draft mode. |
if ((isLl || !Files.exists(objPath)) && | ||
incCompilationContext.shouldCompile(packageName)) { |
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.
We now check upstream if the src
file is newer and/or the obj
file doesn't exist to get here.
Upstream we remove the .ll
so I think we could have .ll.o
laying around.
https://github.com/scala-native/scala-native/blob/main/tools/src/main/scala/scala/scalanative/build/core/ScalaNative.scala#L96
So it seems we should be removing the corresponding .ll.o
files but then I am not sure what the incCompilationContext.shouldCompile(packageName)
is doing anyway. The only way this works currently is that if any source like .c
gets modified we see that and then remove and re-unpack the library thus wiping out the .o
files in the process - otherwise there could be mismatched .c
to .o
.
Any ideas or insight would be appreciated. If this is ok for now we could take this and I could do a followup to deal with any other potential issues.
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.
IncCompilationContext is only comparing the content of .ll files. The .c/.cpp files would be always re-compiled. As far I remember the concept was to split NIR when generating LLVM IR to files corresponding to package names (class-level granularity was giving bad performance results). Hash of the content for all nir Defs within the given package would be stored between compilations. Only in case if the hash would differ we would actually produce .ll file and would compile it. Otherwise, the previous .ll.o file would be used when linking.
Any way it should be safe to remove .o files for C/Cpp sources, but .ll.o files shall remain.
Maybe when adding this feature we have not though about some cases, so probably there is some place for improvements in the future.
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.
Ok, I think I can improve this quite a bit after reading the PR and I will read the report as well. I found that clang
respects the time stamps and doesn't compile files that are not out of date. This PR avoids creating a process to call clang
surmising that the overhead of checking 100s of file stamps is less than creating 100s of processes.
We can do the same for the ll
so I think this can be faster. They only downside for now is that changing settings for optimization that changes the ll
files, like "single" versus "separate" will require a sbt clean
. We can document that in this PR and remove once further improvements remove that restriction.
Once we have mutable private settings and cache the config, we can cleanup based on the settings to avoid any dirty ll
. Reloading the build for setting changes should start the process over so the new settings can take effect and any cleaning due to changes can be done. We also should add a well known name for the "empty" package and also for a "single" ll
just so it is easier for people to figure out.
References:
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.
Ok, I think this should work. First build from clean was [info] Total (4526 ms)
for the native portion.
sbt:scala-native> sandbox2_12/run
[info] Linking (407 ms)
[info] Checking intermediate code (43 ms)
[info] Dumping intermediate code (linked) (176 ms)
[info] Discovered 645 classes and 4316 methods
[info] Optimizing (debug mode) (378 ms)
[info] Checking intermediate code (19 ms)
[info] Dumping intermediate code (optimized) (129 ms)
[info] Dumping intermediate code (lowered) (177 ms)
[info] Generating intermediate code (341 ms)
[info] Produced 36 files
[info] Compiling to native code (109 ms)
[info] Linking native code (immix gc, none lto) (0 ms)
[info] Total (1612 ms)
Hello, World!
[success] Total time: 2 s, completed Oct 28, 2022, 2:44:36 PM
sbt:scala-native> sandbox2_12/run
[info] compiling 1 Scala source to /Users/eric/workspace/scala-native/sandbox/.2.12/target/scala-2.12/classes ...
[info] Linking (373 ms)
[info] Checking intermediate code (25 ms)
[info] Dumping intermediate code (linked) (125 ms)
[info] Discovered 645 classes and 4316 methods
[info] Optimizing (debug mode) (357 ms)
[info] Checking intermediate code (24 ms)
[info] Dumping intermediate code (optimized) (134 ms)
[info] Dumping intermediate code (lowered) (186 ms)
[info] Generating intermediate code (419 ms)
[info] Produced 36 files
[info] Compiling to native code (182 ms)
[info] Linking native code (immix gc, none lto) (102 ms)
[info] Total (1752 ms)
Hello, World2!
With nothing done we get the following. Hopefully, we can avoid that work as well in the future.
sbt:scala-native> sandbox2_12/run
[info] Linking (294 ms)
[info] Checking intermediate code (9 ms)
[info] Dumping intermediate code (linked) (89 ms)
[info] Discovered 645 classes and 4316 methods
[info] Optimizing (debug mode) (307 ms)
[info] Checking intermediate code (18 ms)
[info] Dumping intermediate code (optimized) (127 ms)
[info] Dumping intermediate code (lowered) (189 ms)
[info] Generating intermediate code (332 ms)
[info] Produced 36 files
[info] Compiling to native code (135 ms)
[info] Linking native code (immix gc, none lto) (0 ms)
[info] Total (1321 ms)
Hello, World2!
4405b4d
to
dcca820
Compare
dcca820
to
981dbed
Compare
@WojciechMazur With deterministic |
Remove external decisions to compile and internalize. This allows all build tools to take advantage of the ability. Other optimizations to cache the config and avoiding other work will be addressed in later pull requests.
This specifically addresses the following:
clang
does this already but we avoid all the processes that are created to callclang
On the M1 for compiling sandbox we save maybe 10% by the eyeball method. For linking it takes more time for the second link normally but this goes to 0 from about 100ms. This not a huge saving but along with the
incrementalCompilation
setting on bigger projects this should help.If this passes CI then test drive is welcomed.
Example: