-
Notifications
You must be signed in to change notification settings - Fork 230
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
perf: Slightly optimise object interner for larger codebase #5170
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Naoki Ikeguchi <[email protected]>
db58b64
to
7c5381d
Compare
Hey cool, the bottleneck got hit 😁 How hard was the facepalm when you saw the comment? This is a good simple approach. There are likely more optimal approaches that would be more work (e.g. I had considered sorting keys and then storing the known keys and values in a trie, which I think is effectively what the records and tuples polyfill does). But I don't think this can hurt to improve for now since it seems to help your perf significantly. Let's see what Will thinks. |
@witemple-msft cc @bterlson Hi, could you please review this when you have time? 🙏 This bottleneck still hitting our codebase to slow down openapi3 emitting. Thank you! |
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.
Ack, sorry! I had asked Brian what he thought and then I missed that he already commented. If Brian's good with it, I'm good with it.
I will add a changelog for this today 🙏 |
Signed-off-by: Naoki Ikeguchi <[email protected]>
Head branch was pushed to by a user without write access
@witemple-msft I added a chronus changelog for this change, could you please take a look? |
We have a larger codebase of TypeSpec (total. 17K lines formatted) and found that emitting part is very slow than the compile part, taking 15 seconds avg. on my M3 Pro machine.
After enabling
--cpu-prof
option of Node.js, I found thatintern
function is taking larger part of the framegraph. As the JSDoc says, it's not well optimised yet.This pull request is maybe the first try of the optimisation and slightly improves the execution time of the emitting part. It lowed the execution time from 15 seconds to 6 seconds.
Profiling comparison
← after, before →