-
-
Notifications
You must be signed in to change notification settings - Fork 386
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
Improved compile speed by running multi-threaded library discovery. #2625
Open
cmaglie
wants to merge
28
commits into
arduino:master
Choose a base branch
from
cmaglie:libs_detector_improvements
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmaglie
requested review from
alessio-perugini,
MatteoPologruto,
matthijskooijman and
umbynos
June 5, 2024 10:14
cmaglie
added
type: enhancement
Proposed improvement
topic: build-process
Related to the sketch build process
labels
Jun 5, 2024
cmaglie
force-pushed
the
libs_detector_improvements
branch
2 times, most recently
from
June 5, 2024 10:21
f408819
to
05c4745
Compare
cmaglie
changed the title
Improved compile speed by runnning multi-threaded library discovery.
Improved compile speed by running multi-threaded library discovery.
Jun 5, 2024
cmaglie
force-pushed
the
libs_detector_improvements
branch
from
June 5, 2024 12:29
05c4745
to
8afa7d8
Compare
LGTM |
cmaglie
force-pushed
the
libs_detector_improvements
branch
from
June 13, 2024 20:53
55ee2f5
to
cc15358
Compare
5 tasks
This change is preparatory for next refactorings
This equivalent change moves the 'first' variable closer to his definition and use.
There is no need to duplicate the preprocessResult/Err variables. This also simplifies naming making it more straighforward.
Also fixed the "low memory" warning printer.
The new release of the library allow ignoring the returned error. arduino/go-properties-orderedmap#42
It slightly simplifies code, but also provide the basis for the next commits.
cmaglie
force-pushed
the
libs_detector_improvements
branch
from
August 8, 2024 15:40
cc15358
to
87765a4
Compare
alessio-perugini
approved these changes
Aug 8, 2024
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.
From my POV, is okay. I tested it on some sketches and I can see the improvement.
However, let's wait for some other users to gather feedback. (Just in case some edge cases arise).
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please check if the PR fulfills these requirements
See how to contribute
before creating one)
our contributing guidelines
UPGRADING.md
has been updated with a migration guide (for breaking changes)configuration.schema.json
updated if new parameters are added.What kind of change does this PR introduce?
This is a tentative to improve compile speed by multi-threading the library discovery phase.
How library discovery works
The library discovery is a sequential process, and could be summarized as follows:
gcc -E ...
)Missing include file "xxx.h"
error then:xxx.h
is added to the includes path listThe above loop continues until one of the following happens:
missing include
Could this be multi-threaded?
The main issue here is that each run of
gcc -E ...
requires the includes path list determined by the previous iterations. At first sight, it seems a strictly sequential process.BTW, there are some strategies that we may use:
When a library is added to the queue, we may speculatively start a multithreaded compilation of the next files in the queue assuming no
missing include
errors will be produced. Most of the time this assumption turns out to be true and the time saved could be dramatic.We may leverage the "library detection cache" to spawn a speculative multithreaded compilation of all the files (predicting all the libraries added in the process), assuming that the
missing include
errors would be the same as the previous successful compilation as saved in the "library detection cache". In this case, we reproduce the same compiles as the previous build. The basic assumption is that the libraries detected will not change between compilations.If an unforeseen
missing include
error is detected, the speculative multithreaded compilation must be canceled and restarted (basically wasting the work done ahead of time). In a multi-core system, since the work "ahead" is done in parallel, the worst-case scenario should take nearly the same time as the "classic" non-multithreaded compile.Due to the complexity of these changes, I'll start this as a draft. I want to add integration tests to trigger edge cases before merging.
What is the current behavior?
What is the new behavior?
Compiles should in general be faster than before.
Does this PR introduce a breaking change, and is titled accordingly?
No
Other information