-
Notifications
You must be signed in to change notification settings - Fork 15
Improve build performance #83
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
Improve build performance #83
Conversation
for (const resource of resources) { | ||
const version = resource.version; | ||
console.log('[%s] Unicode v%s', getTime(), version); | ||
console.groupCollapsed(); |
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.
As a bonus, this refactor also fixes another build hazard: Previously when there is an error thrown from the building worker, the cluster main does not exit early, nor does it exit with non-zero code. Now that all building steps are within one process, any thrown errors should invalidate the build step.
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.
I have to revert the remove cluster commit because I overlooked the total time reported by the zsh time. There is still improvement from multiple processes. I will see if I can fix the build hazard directly.
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.
Fixed the mentioned build hazard in c6f9130.
See https://github.com/node-unicode/node-unicode-data/actions/runs/11724848834/job/32659747285 for the error log example.
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.
LGTM to merge after rebase. Thanks a lot for your continued work on this!
In node-unicode#80 we made a performance regression that the bidiMirroringGlyphMap is generated in the property iteration. This is also fixed.
c6c50a9
to
89544f1
Compare
Rebuilding all packages after merging this, I noticed that |
No, this should be a bug. I will open a new PR. |
This PR contains commits from #82. I will rebase once that PR gets merged.
This PR fixed a performance regression introduced in #80 where we generated the bidi_mirroring_glyph data when enumerating the glyph mappings, although the final data is correct, this is apparently not desired.
We also removed the cluster building approach because at this point, it is more efficient to run the V8 optimized code again and again for several seconds than forking multiple V8 processes simutaneously optimizing code which does not run for a very long time.Last we used the native
JSON.stringify
when generating the gzipped data. Because the data is gzipped and not human-readable anyway, there is not very much benefit runningjsesc
before gzip.Unsurprisingly, the last commit generates lots of output diffs. However, they are equivalent for end users, so I didn't attach the output diff in this PR.
Runtime measured in my local environment (Apple M1 Max 10c)
The time spent on each task when processing Unicode 16.0.0 in this PR (Click to unfold)