Stream: git-wasmtime

Topic: wasmtime / PR #4536 cranelift-frontend: Reuse visited blo...

Wasmtime GitHub notifications bot (Jul 26 2022 at 22:50):

fitzgen opened PR #4536 from reuse-hash-set-in-can-optimize-var-lookup to main:

First, we switch from a BTreeSet to a HashSet because clearing a BTreeSet
will deallocate the btree's nodes but clearing a HashSet will not deallocate
the backing hash table, saving the space to reuse for future insertions.

Then, we reuse the same set (and therefore the same allocation) across every
call to can_optimize_var_lookup.

This results in a 1.22x to 1.32x speed up on various Sightglass benchmarks:

compilation :: nanoseconds :: benchmarks/pulldown-cmark/benchmark.wasm

  Δ = 39478181.76 ± 3441880.32 (confidence = 99%)

  main.so is 0.75x to 0.79x faster than reuse-set.so!
  reuse-set.so is 1.27x to 1.32x faster than main.so!

  [160128343 172174751.09 213325968] main.so
  [115055695 132696569.33 200782128] reuse-set.so

compilation :: nanoseconds :: benchmarks/bz2/benchmark.wasm

  Δ = 22576954.88 ± 1830771.68 (confidence = 99%)

  main.so is 0.77x to 0.81x faster than reuse-set.so!
  reuse-set.so is 1.25x to 1.29x faster than main.so!

  [100449245 106820149.65 118628066] main.so
  [77039172 84243194.77 128168647] reuse-set.so

compilation :: nanoseconds :: benchmarks/spidermonkey/benchmark.wasm

  Δ = 664533554.97 ± 22109170.05 (confidence = 99%)

  main.so is 0.81x to 0.82x faster than reuse-set.so!
  reuse-set.so is 1.22x to 1.23x faster than main.so!

  [3549762523 3640587103.35 3798662501] main.so
  [2793335181 2976053548.38 3192950484] reuse-set.so

(Benchmark results for cycles, rather than wall time, are ~identical.)

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jul 26 2022 at 22:50):

fitzgen requested cfallin for a review on PR #4536.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:29):

fitzgen updated PR #4536 from reuse-hash-set-in-can-optimize-var-lookup to main.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:29):

fitzgen requested jameysharp for a review on PR #4536.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:38):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:38):

jameysharp created PR review comment:

Huh, moving the insert after changing current doesn't seem right. Did you mean to do that?

A pre-existing nit: insert returns true if contains would have returned false. So assuming you don't want to change the order here those two calls can be collapsed into if !self.visited.insert(current) { ...

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:42):

fitzgen created PR review comment:

Huh, moving the insert after changing current doesn't seem right. Did you mean to do that?

Have to to appease the borrow checker, since predecessors has a shared borrow of self.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:42):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:43):

fitzgen created PR review comment:

Ah but I did introduce a bug, I see now what you mean, will fix in a sec.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:43):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:46):

fitzgen updated PR #4536 from reuse-hash-set-in-can-optimize-var-lookup to main.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:46):

fitzgen requested jameysharp for a review on PR #4536.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:47):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:47):

jameysharp created PR review comment:

The simplest fix is to move the visited check before the predecessors.len() checks: if we've visited this block before, it already passed the predecessors.len() checks.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:50):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 26 2022 at 23:52):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 27 2022 at 01:38):

fitzgen merged PR #4536.

Last updated: Dec 23 2024 at 12:05 UTC