Wasmtime GitHub notifications bot (Aug 24 2023 at 16:45): Wasmtime GitHub notifications bot (Aug 24 2023 at 16:45):alexcrichton opened PR #6902 from alexcrichton:update-fixups to bytecodealliance:main:
This commit is a follow-up to #6804 with the discussion on #6818. This undoes some of the changes from #6804, such as bringing a size parameter back to
emit_island, and implements the changes discussed in #6818. Namely fixups are now tracked in apending_fixupslist for editing and modification and then during island emission they're flushed into aBinaryHeaptracked asfixup_records. Additionally calculation of the size of the pending island is now done as a single calculation rather than updating metadata as we go along. This is required because fixups are no longer entirely cleared during island emission so the previous logic of "reset the island size to zero" and have it get recalculated as the island is emitted is no longer applicable. I opted to go with a simplistic version of this for now which assumes that all veneers are the worst case size, but if necessary I think this could be more optimal by tracking each veneer in a counter.Closes #6818
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Aug 24 2023 at 16:45):alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6902.
Wasmtime GitHub notifications bot (Aug 24 2023 at 16:45):alexcrichton requested fitzgen for a review on PR #6902.
Wasmtime GitHub notifications bot (Aug 24 2023 at 18:09):fitzgen requested cfallin for a review on PR #6902.
Wasmtime GitHub notifications bot (Aug 24 2023 at 18:20):cfallin submitted PR review:
Thanks for taking this on!
The new version looks correct; I have some thoughts on performance below.
Wasmtime GitHub notifications bot (Aug 24 2023 at 18:20):cfallin submitted PR review:
Thanks for taking this on!
The new version looks correct; I have some thoughts on performance below.
Wasmtime GitHub notifications bot (Aug 24 2023 at 18:20):cfallin created PR review comment:
I'm worried this will lead to a new kind of quadratic behavior: we're iterating over all pending fixups every time we check whether an island is needed, which happens (at least) once per basic block. In the common case (no islands)
pending_fixup_recordswill contain all fixups in the function -- every block-label reference in branches, every constant-label reference.Perhaps we could keep a running deadline for
pending_fixup_recordsthat is updated as we push new ones and cleared (set to max distance) at island emission time? It's a pessimization compared to this code because it doesn't allow loosening the deadline if we chomp a branch; but in practice that's unlikely to make any difference because the branch kind that we chomp (common conditional branches) will occur frequently and we do keep many/most of them.
Wasmtime GitHub notifications bot (Aug 24 2023 at 18:20):cfallin created PR review comment:
Rather than populate the fixup-records heap and then drain it in order, could we do a two-stage thing where we process all pending fixups, push those that are still pending (not resolvable) into the heap, and then iterate over those?
I'm worried that this will somewhat noticeably pessimize the common case: one run of this algorithm at the end of a function that had no islands otherwise, where everything can be resolved. In that case, we shouldn't pay the cost to work out deadlines and sort by them.
Wasmtime GitHub notifications bot (Aug 24 2023 at 22:30):alexcrichton updated PR #6902.
Wasmtime GitHub notifications bot (Aug 24 2023 at 22:31):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 24 2023 at 22:31):alexcrichton created PR review comment:
Sounds good to me! That was actually preexisting as well that if we chomp things we don't update the deadline, and that seems fine to me yeah
Wasmtime GitHub notifications bot (Aug 24 2023 at 23:49):cfallin submitted PR review:
LGTM!
Wasmtime GitHub notifications bot (Aug 25 2023 at 00:37):cfallin merged PR #6902.
Last updated: Dec 23 2024 at 12:05 UTC