Don't force veneers on island emission#6902
Merged
cfallin merged 3 commits intobytecodealliance:mainfrom Aug 25, 2023
Merged
Conversation
This commit is a follow-up to bytecodealliance#6804 with the discussion on bytecodealliance#6818. This undoes some of the changes from bytecodealliance#6804, such as bringing a size parameter back to `emit_island`, and implements the changes discussed in bytecodealliance#6818. Namely fixups are now tracked in a `pending_fixups` list for editing and modification and then during island emission they're flushed into a `BinaryHeap` tracked as `fixup_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 bytecodealliance#6818
Member
|
Redirecting review to @cfallin since I'm not super familiar with these bits of code. |
cfallin
reviewed
Aug 24, 2023
Member
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks for taking this on!
The new version looks correct; I have some thoughts on performance below.
github-actions
bot
added
cranelift
Update this as pending fixups are added and then clear this out when islands are emitted.
Instead process them immediately if they're ready.
eduardomourar
pushed a commit
to eduardomourar/wasmtime
that referenced
this pull request
Sep 6, 2023
* Don't force veneers on island emission This commit is a follow-up to bytecodealliance#6804 with the discussion on bytecodealliance#6818. This undoes some of the changes from bytecodealliance#6804, such as bringing a size parameter back to `emit_island`, and implements the changes discussed in bytecodealliance#6818. Namely fixups are now tracked in a `pending_fixups` list for editing and modification and then during island emission they're flushed into a `BinaryHeap` tracked as `fixup_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 bytecodealliance#6818 * Keep a running count for pending fixup deadline Update this as pending fixups are added and then clear this out when islands are emitted. * Don't force all fixups to go through a binary heap Instead process them immediately if they're ready.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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