Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork435
Improve the performance of includes scanning#1735
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
CLAassistant commentedMay 18, 2022
Sawyer seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, pleaseadd the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let usrecheck it. |
facchinm commentedMay 23, 2022
I'm testing this PR and the speedup is amazing 🤩 |
matthijskooijman commentedMay 24, 2022
@shaoyie, can you expand a little bit more on how this changes the include scanning process? The old caching code was intended to ensure that the results are always the same, with or without caching. I haven't dug into your new code deeply yet, but I have the impression that this might end up with different results depending on goroutine timing in some cases (I'm thinking of a case where one library exposes In any case, speeding up this process is much welcomed, since it indeed is quite slow currently. For a related (but distinctly different) improvement to this process, seearduino/arduino-builder#217, which improves caching when compile errors are present by letting the include caching generate .d files (in addition to the compilation process). I never finished that PR properly, since it probably requires changes to recipes/platform.txt, but I never took the time to figure out what is needed exactly and how to do that in a backward compatible way. As I said, it's distinct from this improvement, but if you're interested in also working on that, feel free to work from my code. |
shaoyie commentedMay 27, 2022
Your impression is right that in multiple goroutines situation, the scanning result may look different depends on the goroutine execution order. |
shaoyie commentedMay 27, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Just like stable sort and unstable sort, the result may vary but legal, depends on what is your expectation. |
| } | ||
| // Loop until all files are handled | ||
| for (!sourceFilePaths.Empty()||unhandled!=0 ) { |
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.
Isn't thisunhandled variable redundant here? the WaitGroup should already take care of waiting for all the processes to finish.
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.
Theunhandled is not redundant here. During the scanning process, we may find new source files. For example, suppose we have only 4 files to scan in the beginning, but we have 8 cores. Then the sourceFilePaths will be empty after the first goroutines fetch their task and the loop will be ended. But during the scanning against the 4 files may append more new found files to the sourceFilePaths but they won't be handled. The variableunhandled is to make sure sourceFilePaths is empty only after all goroutines finish the jobs.
| // The first source file is the main .ino.cpp | ||
| // handle it first to setup environment for other files | ||
| findIncludesUntilDone(ctx,cache,sourceFilePaths.Pop()) |
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.
Why themain.ino.cpp is processed separately? If the processing order of the files doesn't matter, this should not be needed.
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.
In my original design, I also think the main.ino.cpp has no difference with other files. And it works in most cases.
But for some special cases, e.g. from the commits history of this PR we can see, the test case
assert run_command(["compile", "-b", "adafruit:samd:adafruit_feather_m4", sketch_path])
in test_compile_part_1.py will fail.
Due to the library found mechanism in arduino-cli, have to set main.ino.cpp as the root of the scanning, then start other scanning from it.
| if(ok) { | ||
| // Ignore the pre-added folder | ||
| if(!strings.HasPrefix(header,"no_resolve")) { | ||
| library,imported:=ResolveLibrary(ctx,header) | ||
| iflibrary==nil { | ||
| if!imported { | ||
| // Cannot find the library and it is not imported, is it gone? Remove it later | ||
| entryToRemove=append(entryToRemove,header) | ||
| cache.valid=false | ||
| } | ||
| }else { | ||
| // Add this library to the list of libraries, the | ||
| // include path and queue its source files for further | ||
| // include scanning | ||
| ctx.ImportedLibraries=append(ctx.ImportedLibraries,library) | ||
| // Since it is already in cache, append the include folder only | ||
| appendIncludeFolderWoCache(ctx,header,library.SourceDir) | ||
| sourceDirs:=library.SourceDirs() | ||
| for_,sourceDir:=rangesourceDirs { | ||
| queueSourceFilesFromFolder(ctx,ctx.CollectedSourceFiles,library,sourceDir.Dir,sourceDir.Recurse) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| returntrue | ||
| }) |
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 I read this correctly, you're basically reusing all the previous set of include paths, but without keeping the history of how you obtained that set. This won't work in all cases, in particular, if the cache is invalidated you must redo all the work again to be sure to not pick libraries that are not needed anymore.
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.
Yes, now the strategy only cares missing files but doesn't care the redundant entries in cache in case it won't break the build.
In the particular case you mentioned, if need to clear the useless libraries, a clean build is required to do the job. And most of the compiling tool have "Clean build" besides "Build", this case can utilize the differences between these two kind of builds.
Not sure is there a better way to handle this case so we can have a better balance between the performance and accurate.
cmaglie commentedAug 8, 2024
Superseded by#2625. |
Uh oh!
There was an error while loading.Please reload this page.
Please check if the PR fulfills these requirements
before creating one)
our contributing guidelines
UPGRADING.mdhas been updated with a migration guide (for breaking changes)Enhancement
It's very slow to scan the includes for a large project when cache is invalid. And the cache is very easy to be invalidated because it caches the includes by a strict chain. Also because of the chain, the scanning can only be performed by single thread.
By applying the sync.Map instead of the array, we can introduce in the goroutine to improve the performance far more better.
titled accordingly?
No breaking
Seehow to contribute