Skip to content

Cranelift: consider heap's guard pages when legalizing heap_addr#5335

Merged
fitzgen merged 3 commits intobytecodealliance:mainfrom
fitzgen:heap-addr-guard-pages
Nov 29, 2022
Merged

Cranelift: consider heap's guard pages when legalizing heap_addr#5335
fitzgen merged 3 commits intobytecodealliance:mainfrom
fitzgen:heap-addr-guard-pages

Conversation

@fitzgen
Copy link
Copy Markdown
Member

@fitzgen fitzgen commented Nov 28, 2022

Fixes #5328

FYI, I opted not to mess with or dedupe the similar logic in cranelift-wasm since that stuff is going to go away soon-ish when we remove heap_addr and collect all this logic into heap_{load,store}. Excited to get to a point where this logic isn't spread across crates and straddling interfaces.

@fitzgen fitzgen requested a review from alexcrichton November 28, 2022 23:26
@fitzgen fitzgen mentioned this pull request Nov 28, 2022
Closed
@jameysharp
Copy link
Copy Markdown
Contributor

jameysharp commented Nov 28, 2022

FWIW, I looked this over and I think it makes sense, but I'd definitely prefer to have somebody else's review on it.

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Nov 28, 2022
cfallin
cfallin approved these changes Nov 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Looks correct to me. One note below on possibly expanding the explanation a bit.

alexcrichton
alexcrichton approved these changes Nov 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

It looks like the prior PR accidentally enabled bounds checks by default in Wasmtime, so I'm sort of surprised we didn't catch that until now. Does sightglass show regressions here or improvements after this commit? If so it might be good to use sightglass on future changes to heap_{addr,store,load} to double-check the intended effect.

@fitzgen
Copy link
Copy Markdown
Member Author

fitzgen commented Nov 29, 2022

It looks like the prior PR accidentally enabled bounds checks by default in Wasmtime, so I'm sort of surprised we didn't catch that until now. Does sightglass show regressions here or improvements after this commit? If so it might be good to use sightglass on future changes to heap_{addr,store,load} to double-check the intended effect.

Yeah would be great if we had nightly sightglass benchmarks set up. I set up some smoke tests for heap_addr legalization with vs without sufficient guard pages to avoid bounds checks (should tide us over until heap_addr is gone).

Sightglass results for this PR:

execution :: instructions-retired :: benchmarks/spidermonkey/benchmark.wasm

  Δ = 3362887399.64 ± 42143.94 (confidence = 99%)

  with-guard-pages.so is 2.05x to 2.05x faster than main.so!

  [6566493438 6566656053.70 6567012773] main.so
  [3203651702 3203768654.06 3204149983] with-guard-pages.so

execution :: instructions-retired :: benchmarks/bz2/benchmark.wasm

  Δ = 253455489.92 ± 1.49 (confidence = 99%)

  with-guard-pages.so is 1.79x to 1.79x faster than main.so!

  [573123166 573123173.64 573123183] main.so
  [319667678 319667683.72 319667699] with-guard-pages.so

execution :: instructions-retired :: benchmarks/pulldown-cmark/benchmark.wasm

  Δ = 16418573.09 ± 99.14 (confidence = 99%)

  with-guard-pages.so is 1.62x to 1.62x faster than main.so!

  [42813022 42813207.84 42813926] main.so
  [26394395 26394634.75 26395352] with-guard-pages.so

compilation :: instructions-retired :: benchmarks/bz2/benchmark.wasm

  Δ = 321257.88 ± 54303.80 (confidence = 99%)

  with-guard-pages.so is 1.11x to 1.15x faster than main.so!

  [2784403 2856173.33 3298299] main.so
  [2460682 2534915.45 3047605] with-guard-pages.so

compilation :: instructions-retired :: benchmarks/spidermonkey/benchmark.wasm

  Δ = 25520928.63 ± 108236.81 (confidence = 99%)

  with-guard-pages.so is 1.12x to 1.12x faster than main.so!

  [232258160 232762746.46 233555099] main.so
  [206468796 207241817.83 207933331] with-guard-pages.so

compilation :: instructions-retired :: benchmarks/pulldown-cmark/benchmark.wasm

  Δ = 676224.25 ± 51124.52 (confidence = 99%)

  with-guard-pages.so is 1.07x to 1.08x faster than main.so!

  [9483066 9609230.75 10037142] main.so
  [8828280 8933006.50 9376518] with-guard-pages.so

@fitzgen fitzgen enabled auto-merge (squash) November 29, 2022 19:41
@alexcrichton
Copy link
Copy Markdown
Member

alexcrichton commented Nov 29, 2022

Nah I don't think it's worthwhile to add more than what's already here given that this is all intended to be short-lived anyway. It's good though that sightglass clearly detects this either way!

@fitzgen fitzgen merged commit 913a2ec into bytecodealliance:main Nov 29, 2022
@fitzgen fitzgen deleted the heap-addr-guard-pages branch November 29, 2022 19:55
@cfallin cfallin mentioned this pull request Dec 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichton alexcrichton alexcrichton approved these changes

@cfallin cfallin cfallin approved these changes

Assignees

No one assigned

Labels

cranelift Issues related to the Cranelift code generator

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Compiling a 8 MB WASM file using Wasmtime dev build takes 20x as long as with Wasmtime 3.0.0

4 participants

@fitzgen @jameysharp @alexcrichton @cfallin