Skip to content

Remove libsqlite from MINIMAL build #23900

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

Merged
merged 1 commit into from
Mar 12, 2025
Merged

Remove libsqlite from MINIMAL build #23900

merged 1 commit into from
Mar 12, 2025

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented Mar 11, 2025

libsqlite is really slow building, and causes the MINIMAL build step to be much slower (especially since it's not parallel).
The testers that use frozen caches also do not depend on it, as they use the ALL library target.
This has the effect that it pushes building this library from the fast/highly parallel build-linux builder to the test builders, and makes build-linux faster.

@dschuff dschuff marked this pull request as draft March 11, 2025 23:30
@sbc100
Copy link
Collaborator

sbc100 commented Mar 11, 2025

Many of the bots run in EM_FROZEN_CACHE mode so you would also need to make sure the tests that use this library are also disabled on those bots.

@dschuff
Copy link
Member Author

dschuff commented Mar 11, 2025

I think that should actually be ok, because any test bot that depends on one of the library modes that we only build MINIMAL instead of ALL for (e.g. wasm64) already run without frozen cache.

@dschuff
Copy link
Member Author

dschuff commented Mar 12, 2025

This does reduce the build-linux bot time by about 5 minutes. I will also look at the effect on the wasm64 tester bots
The overall CPU time will not improve significantly (since sqlite will have to be built on the individual tester bots), but the overall wallclock time might improve because the build-linux bot has better parallelism after this PR (the 5-minute improvement comes mostly from removing the serialization bottleneck rather than the actual time spent building the library). There might also be a small cost savings since the build-linux bot has 4x more expensive and is now better utilized.
And time-to-test-failure might improve since all the bots that depend on build-linux will now trigger 5 minutes sooner.

I'm thinking small speed/cost improvement, small DevX improvement, probably worth it.
WDYT?

@sbc100
Copy link
Collaborator

sbc100 commented Mar 12, 2025

Sounds good. If a library isn't used on the FROZEN_CACHE bots I don't think its belongs on this list.

@dschuff dschuff marked this pull request as ready for review March 12, 2025 00:21
@dschuff
Copy link
Member Author

dschuff commented Mar 12, 2025

It turns out that the test-wasm64 from the most recent main build is actually building libsqlite3-mt on the tester instead of using the cached one. I'm not sure what's going on there, it suggests something isn't right. In any case despite the fact that the wasm64 tester on this PR is now building libsqlite3.a instead of using the cache, the total runtime difference for test-wasm64 is in the noise, so I think this should be good to land.

@dschuff dschuff merged commit 53b38d0 into main Mar 12, 2025
27 of 31 checks passed
@dschuff dschuff deleted the nosql branch March 12, 2025 00:31
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.

2 participants