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

perf(semantic): initialize builder storage with pre-allocated capacity #4332

Closed
wants to merge 4 commits into from

Conversation

DonIsaac
Copy link
Contributor

What This PR Does

Initializes nodes, scopes, symbols, and several other internal stacks with some capacity already allocated. This should reduce the number of re-allocations Semantic needs to make while traversing a program.

@DonIsaac DonIsaac added A-semantic Area - Semantic C-performance Category - Solution not expected to change functional behavior, only performance labels Jul 17, 2024
Copy link

graphite-app bot commented Jul 17, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

Copy link

codspeed-hq bot commented Jul 17, 2024

CodSpeed Performance Report

Merging #4332 will degrade performances by 6.1%

Comparing don/semantic/perf/new-with-capacity (308fe78) with main (db2fd70)

Summary

⚡ 7 improvements
❌ 2 regressions
✅ 23 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main don/semantic/perf/new-with-capacity Change
semantic[RadixUIAdoptionSection.jsx] 109.1 µs 116.2 µs -6.1%
semantic[antd.js] 175.5 ms 163.2 ms +7.49%
semantic[cal.com.tsx] 57.6 ms 55.3 ms +4.17%
semantic[checker.ts] 105.2 ms 90 ms +16.8%
semantic[pdf.mjs] 26.8 ms 25.7 ms +4.3%
transformer[RadixUIAdoptionSection.jsx] 232.5 µs 241.8 µs -3.86%
transformer[antd.js] 298.5 ms 286.2 ms +4.28%
transformer[cal.com.tsx] 75.6 ms 73.3 ms +3.19%
transformer[checker.ts] 171.2 ms 157.1 ms +8.98%

@DonIsaac
Copy link
Contributor Author

DonIsaac commented Jul 17, 2024

@overlookmotel any insight into why this makes some benchmarks perform better while others perform worse? I think it has to do with reserving 8 slots for symbols? And maybe partially due to it being a small file?

@overlookmotel
Copy link
Contributor

@DonIsaac Ha! By chance, Dunqing has been working on exactly the same thing today, but taking a slightly different approach - #4328. It's unfortunate his PR title is quite mysterious "test: visit" so you didn't see it - I think he intended it initially as a quick temporary test.

But... actually it's quite useful you've investigated this other heuristic approach because Dunqing's approach has run into same problem you have here with RadixUIAdoptionSection.jsx.

Let's join forces!

I'll post more comment on #4328 to continue the conversation in one place.

@DonIsaac
Copy link
Contributor Author

Love it! LMK if you're reachable on Discord as well.

CC: @Dunqing

function_stack: Vec::with_capacity(4),
// Most TS modules are not nested, and those that are are usually only 1 level deep.
namespace_stack: Vec::with_capacity(2),
nodes: AstNodes::with_capacity(source_text.len() >> 4),
Copy link
Contributor

@overlookmotel overlookmotel Jul 19, 2024

Choose a reason for hiding this comment

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

@DonIsaac Just a side note: There's no need to write >> 4 here. When you multiply or divide by a power of 2, the compiler will convert that to a bitshift operation. So writing / 16 is preferable, as it's easier to read for us humans, and results in same assembly.

@DonIsaac DonIsaac closed this Jul 20, 2024
@DonIsaac DonIsaac deleted the don/semantic/perf/new-with-capacity branch July 20, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semantic Area - Semantic C-performance Category - Solution not expected to change functional behavior, only performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants