Improve the performance of includes scanning #1735
Conversation
|
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, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
per1234
added
type: enhancement
|
I'm testing this PR and the speedup is amazing 🤩 |
|
@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, see arduino/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. |
Your impression is right that in multiple goroutines situation, the scanning result may look different depends on the goroutine execution order. |
|
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.
Isn't this unhandled variable redundant here? the WaitGroup should already take care of waiting for all the processes to finish.
There was a problem hiding this comment.
The unhandled 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 variable unhandled 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.
Why the main.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.
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) | ||
| if library == 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 := range sourceDirs { | ||
| queueSourceFilesFromFolder(ctx, ctx.CollectedSourceFiles, library, sourceDir.Dir, sourceDir.Recurse) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return true | ||
| }) |
There was a problem hiding this comment.
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.
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.
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
See how to contribute