perf(semantic): store unresolved refs in a stack

[lucab]

This tweaks SemanticBuilder logic in order to accumulate unresolved
references in a stack, getting rid of the previous index-vector which
is not required under the current access pattern.

Ref: #4107 (comment)

[graphite-app]

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.

[lucab]

• perf(semantic): store unresolved refs in a stack #4162 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @lucab and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #4162 will improve performances by 4.44%

_{Comparing ups/semantics-refs-stack (2203143) with main (f2b3273)}

Summary

⚡ 5 improvements
✅ 24 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	ups/semantics-refs-stack	Change
⚡	semantic[antd.js]	212.1 ms	205.9 ms	+3.03%
⚡	semantic[checker.ts]	136.3 ms	130.5 ms	+4.44%
⚡	semantic[pdf.mjs]	33.3 ms	32 ms	+3.99%
⚡	transformer[checker.ts]	203 ms	195.4 ms	+3.87%
⚡	transformer[pdf.mjs]	53 ms	51.2 ms	+3.63%

[overlookmotel]

Nice work! However, I think we can potentially do even better.

What I had in mind is that you never call unresolved_references.pop(). Instead, hold on to the hash map and re-use it. That way we avoid generating (and allocating) new hash map when entering a scope most of the time.

unresolved_references is Vec<UnresolvedReferences>, indexed by scope depth.

e.g. with this input:

function foo() {}
function bar() {}

• Enter top level scope:
  • Depth is 0.
  • Push a UnresolvedReferences hash map to unresolved_references.
  • Current hashmap is unresolved_references[0].
• Enter scope for foo:
  • Increment depth to 1.
  • Push another UnresolvedReferences hash map to unresolved_references.
  • Current hashmap is unresolved_references[1].
• Exit scope for foo:
  • Clear current hashmap (unresolved_references[1]).
  • Decrement depth to 0.
  • Do NOT pop from unresolved_references.
  • Current hashmap is now unresolved_references[0].
• Enter scope for bar:
  • Increment depth to 1.
  • Do NOT push to unresolved_references.
  • Current hashmap is unresolved_references[1] (i.e. reuse same hash map that was used for foo)
• Exit scope for bar: same as for foo
• Exit top level scope: same

Does this make any sense? I think it'll be a further perf boost due to reducing allocations, and reusing allocations which are already warm in the cache.

[lucab]

@overlookmotel I see, you want to reuse previously allocated hashmaps (sorry I missed that point from your original request). Do you mind if I try to do that in a separate PR after landing this one, so that it's easier to see the performance delta?

[overlookmotel]

Do you mind if I try to do that in a separate PR after landing this one

Yes, sure. Whichever way you prefer. In any case, this PR is already a really nice perf boost. Also... feel free not to do it at all! It's completely up to you what you want to work on. I'm just making suggestions, not commands!

[overlookmotel]

If you're happy with this PR, let me know and I'll merge.

[lucab]

Yes, this one is ready to merge.

[graphite-app]

Merge activity

• Jul 10, 10:44 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Jul 10, 10:46 AM EDT: Boshen added this pull request to the Graphite merge queue.
• Jul 10, 11:11 AM EDT: Boshen merged this pull request with the Graphite merge queue.