GenerateHeaderFrom{Json,Raw}.cmake
#30842
This PR aims to reduce build time by replacing multiple file(WRITE|APPEND ...) commands with a single file(WRITE ...) command.
Due to differences in implementation (e.g., filesystem design, system calls, caching), a noticeable improvement in build time is observed only on Windows.
Additionally, the code has been refactored to remove the remainder local variables.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | sipsorcery, TheCharlatan, maflcko |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
I am not measuring a difference in performance in the generation of the headers between master and this PR.
Building on Windows benefits most.
I am not measuring a difference in performance in the generation of the headers between master and this PR.
Building on Windows benefits most.
On my Windows machine, the time to process data/*.h headers for the test_bitcoin target has been reduced from ~155s to ~105s.
@sipsorcery What do you think?
@sipsorcery What do you think?
The overall build time for me on Win11 msvc reduced by 44s (only one test run each of the master and PR builds).
master:
0PS C:\Dev\github\bitcoin> Measure-Command { cmake --build build --config Release }
1TotalSeconds : 637.2643505
pr 30842:
0PS C:\Dev\github\bitcoin> Measure-Command { cmake --build build --config Release }
1TotalSeconds : 553.3287435
IMHO it’s always beneficial to improve CI times so +1 for me.
tACK b07fe666f27e2b2515d2cb5a0339512045e64761
This PR aims to reduce the build time.
Can you update the PR description to clarify that this is only relevant for native windows builds. Can you also add the explanation for why this only makes a difference on Windows.
This PR aims to reduce the build time.
Can you update the PR description to clarify that this is only relevant for native windows builds. Can you also add the explanation for why this only makes a difference on Windows.
I did my best.
review ACK b07fe666f27e2b2515d2cb5a0339512045e64761
Seems fine to always buffer everything in memory twice, because it is already buffered in memory once (as the full hex_content) and the data should be small (generally less than 10 MB?)
Not sure why, but for me the command took almost an hour:
0$ ps -p 816447 --format time,rss,cmd
1 TIME RSS CMD
200:51:12 117640 /usr/bin/cmake -DRAW_SOURCE_PATH=_/src/bench/data/block413567.raw -DHEADER_PATH=_/bld-cmake/src/bench/data/block413567.raw.h -DRAW_NAMESPACE=benchmark::data -P _/cmake/script/GenerateHeaderFromRaw.cmake
Previously it took only a few seconds.
Not sure why, but for me the command took almost an hour:
0 1$ ps -p 816447 --format time,rss,cmd 2 3 TIME RSS CMD 4 500:51:12 117640 /usr/bin/cmake -DRAW_SOURCE_PATH=_/src/bench/data/block413567.raw -DHEADER_PATH=_/bld-cmake/src/bench/data/block413567.raw.h -DRAW_NAMESPACE=benchmark::data -P _/cmake/script/GenerateHeaderFromRaw.cmakePreviously it took only a few seconds.
What’s your system?