Skip to content

perf(parser): mark error paths as cold#10614

Merged
Boshen merged 1 commit intooxc-project:mainfrom
DonIsaac:don/parser/perf/cold-errors
Apr 25, 2025
Merged

perf(parser): mark error paths as cold#10614
Boshen merged 1 commit intooxc-project:mainfrom
DonIsaac:don/parser/perf/cold-errors

Conversation

@DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Apr 25, 2025

What This PR Does

Adds #[cold] to parser functions that record errors.

I'm making a draft for now to run benchmarks and see if my hypothesis is correct. Locally I saw some improvement, but my machine gets finicky when it heats up, so it could be noise.

Hypothesis

LLVM will consider call sites of cold functions to also be cold (assuming that the function is always called). By marking error recording functions as #[cold], if statements that check for diagnostics should be considered cold themselves, thus hinting to branch predictors that the happy path is more likely. This should give branch prediction a higher success rate and lower the odds the CPU's pipeline gets thrown out.

e.g.

fn parse_foo(&self) -> FooNode {
  if some_bad_thing {
    // this block post-dominates call to `error`, suggesting that it won't be called
    // frequently
    self.error(diagnostics::bad_thing_happened());
  }
  // happy path continues
}

@graphite-app
Copy link
Contributor

graphite-app bot commented Apr 25, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@github-actions github-actions bot added A-parser Area - Parser C-performance Category - Solution not expected to change functional behavior, only performance labels Apr 25, 2025
@Boshen
Copy link
Member

Boshen commented Apr 25, 2025

I tried in #10599, but I think it makes sense to merge this change.

I just recalled codspeed doesn't weigh in branch predictions, so we need to rely on wall clock benchmark + cpu instrustions.

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 25, 2025

CodSpeed Instrumentation Performance Report

Merging #10614 will not alter performance

Comparing DonIsaac:don/parser/perf/cold-errors (7f18ca4) with main (fae8c26)

Summary

✅ 36 untouched benchmarks

@DonIsaac DonIsaac marked this pull request as ready for review April 25, 2025 04:39
@Boshen Boshen merged commit 7059ffa into oxc-project:main Apr 25, 2025
27 checks passed
@DonIsaac DonIsaac deleted the don/parser/perf/cold-errors branch April 25, 2025 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-parser Area - Parser 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