Skip to content
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

Optimization: Preprocess files in parallel #653

Closed
wants to merge 3 commits into from
Closed

Optimization: Preprocess files in parallel #653

wants to merge 3 commits into from

Conversation

sylt
Copy link
Contributor

@sylt sylt commented Jan 7, 2022

For large projects having many header files, I've seen very positive results: When running the preprocessing in parallel on the project I am working on, I saw that our "ceedling test:all" command went from taking 45 seconds (with 1 compile_thread) to 12 seconds (with 8 compile threads).

Bonus: When doing stuff in parallel, don't spawn more threads than necessary. This change isn't strictly necessary, but I thought that it made sense.

For large projects having many header files, I've seen very positive results.
@mvandervoord
Copy link
Member

@sylt -- we're reworking the core engine to run parallelization better. If you have some time, would you be interested in playing with https://github.com/ThrowTheSwitch/Ceedling/tree/feature/decentralize_config and contributing to that branch instead? The goal is to replace the main branch with this engine.

@sylt
Copy link
Contributor Author

sylt commented Jan 7, 2022

@mvandervoord: Alright, thanks for the information, I have had a superficial look through the changes done on the branch you referred to. Based on what I think I saw, I'm not certain whether this change is applicable on that branch, since I believe an equivalent change already has been done (by you!). I'm basing this on that in test_invoker.rb:setup_and_invoke(), we are now doing preprocessing in parallel. I think the relevant parts were added in this commit to the work branch.

I think we can conclude that it is more than likely that this change gets superseded by the work you are doing on decentralize_config. I'm not sure about how much work remains for the feature branch -- perhaps there is a list of remaining issues somewhere here on github? I just thought that with the remaining work in mind, it still might be worth to consider merging this change, as it should provide significant improvements right now.

On a related note, I saw that the CI tests unfortunately failed. That's a pity. When I run bundle exec rake locally on my computer, all 238 tests pass. I'm not running an Ubuntu docker image however, I run directly on Debian Unstable, if that has any importance.

@sylt
Copy link
Contributor Author

sylt commented Jan 7, 2022

Regarding the CI, it seems as if it's running Ruby 3.1.0, which I guess isn't the expected version based on the YAML file. I myself am running 2.7.

@sylt
Copy link
Contributor Author

sylt commented Jan 7, 2022

Regarding the wrong Ruby version used, I tried to make a fix in #654. With some luck, it should fix the pipeline here as well.

@rossb93
Copy link

rossb93 commented Jan 20, 2023

Is this something that could be pulled in master? Ceedling is fantastic but even at only 200 odd tests it takes about 3-4 mins to run, mostly due to have to compile the mocks each time.

@mvandervoord
Copy link
Member

Replaced with large-scale parallelization in PR #739

@sylt
Copy link
Contributor Author

sylt commented Aug 23, 2023

@mvandervoord While I do understand that PR #739 is better in many ways, this PR does provide quite a significant speed-up for large projects without being very intricate. If now it takes more time than anticipated to get #739 production ready, perhaps it could be worth merging this anyway since it would be an improvement to the users of Ceedling? Well, unless you see there would be a risk of regressions of course (for me though in my test set up, I haven't seen any so far).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants