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

fix(common): set consistent timestamps on template-generated files #3353

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

matejcik
Copy link
Contributor

this helps the build system keep track of whether anything has actually changed

the important part is lines 154-156, the rest is fluff

cc @cepetr, I suspect that the bad rebuild times with scons could be helped a lot by finding & fixing bugs like this one

@matejcik
Copy link
Contributor Author

matejcik commented Nov 1, 2023

comment from 795d14c:

some pretty safe & reasonable defaults to speed up SCons:

  • look at timestamps to figure out whether a file has changed
  • but only if the timestamp is older than 10 secs (which should avoid any problems with 1s precision of mtime, as well as clock drifts etc)
  • implicit cache is something to do with parsing C files for dependencies

see https://github.com/SCons/scons/wiki/GoFastButton

prusnak
prusnak previously requested changes Nov 3, 2023
core/Makefile Outdated
@@ -1,6 +1,6 @@
.PHONY: vendor

JOBS = 4
JOBS = $(shell nproc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not portable, nproc does not exist on macOS and most probably other non-GNU systems

Quick fix would be:

Suggested change
JOBS = $(shell nproc)
JOBS = $(shell nproc || echo 4)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this work? sysctl -n hw.ncpu
i'm happy to do a switch for darwin for this

Copy link
Member

@prusnak prusnak Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it works on Darwin, but it is not ideal for Apple Silicon machines. Apple Silicon machines are ARM big.LITTLE and nproc reports the number of all physical cores (both Performance and Efficiency), but the compilation performs better if only number of only Performance cores is used.

What I've seen in other projects was they use the following (in order of priority):

  • hw.perflevel0.physicalcpu
  • hw.physicalcpu
  • hw.ncpu

which translates to

sysctl -n hw.perflevel0.physicalcpu || sysctl -n hw.physicalcpu || sysctl -n hw.ncpu

If we want to be 100% perfect, we could use this beast:

sysctl -n hw.perflevel0.physicalcpu 2>/dev/null || sysctl -n hw.physicalcpu 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || nproc 2>/dev/null || echo 2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hannsek Hannsek requested a review from cepetr January 10, 2024 12:47
@cepetr
Copy link
Contributor

cepetr commented Jan 26, 2024

I've done some evaluation (on my machine), and here are results:

build type before after
build_unix / all files 25.5s 19s
build_unix / no changes 2.5s 2.5s
build_unix / 1 .c file changed 11.5s 2.5s
build_unix / 1 .rs file changed 3s 3 s
build_firmware / all files 38s 35s
build_firmware / no changes 10.5s 2.5s
build_firmware / 1 .c file changed 19.5s 4s
build_firmware / 1 .rs file changed 11s 10s

It looks great.

this helps the build system keep track of whether anything has actually changed
some pretty safe & reasonable defaults to speed up SCons:

- look at timestamps to figure out whether a file has changed
- but only if the timestamp is older than 10 secs (which should avoid any problems with 1s precision of mtime, as well as clock drifts etc)
- implicit cache is something to do with parsing C files for dependencies

see https://github.com/SCons/scons/wiki/GoFastButton
Run the preprocessor on each file separately.
This allows parallelization and doesn't need to re-run for all files if just a small number is changed.

Replace simple extractors with one-liners which are generally going to be faster.
@matejcik matejcik merged commit 7958061 into main Feb 1, 2024
56 of 59 checks passed
@matejcik matejcik deleted the matejcik/template-timestamps branch February 1, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants