Skip to content

Conversation

kripken
Copy link
Member

@kripken kripken commented Jan 31, 2024

Followup to #6243 which handled empty ones.

@kripken kripken requested a review from tlively January 31, 2024 19:40
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Would it be worth continuing to split these possibly-trapping active segments, but ensure that their last chunks are not elided? Then the initialization of the last chunk (or a previous chunk) would trap iff the original segment would have trapped. Or would it be a problem that this would cause partial initialization to become visible before the trap? I always forget whether we care about side effects before a trap.

@kripken
Copy link
Member Author

kripken commented Jan 31, 2024

Interesting idea! I hadn't thought of that.

I think this would be technically incorrect because of partial initialization. At least that was the case in the bulk memory proposal ("bounds checks are done before any writes") - I'm not sure offhand where to read the spec for active segments, but surely we didn't introduce a difference there..?

However it might be an incorrectness we can live with - we already live with reordering traps, for example (which have different stack traces, which is observable). But I think it would be more complicated to do this idea - we'd need to track split-off parts of the original segment etc., and I think for basically no benefit - no real-world code has trapping segments (I hope).

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

LGTM. We definitely don't need to care about modules that definitely trap, but it's more realistic that there might be modules that use a global as the offset, in which case it would be unfortunate if we did not optimize. (Do shared libraries already do this?)

Comment on lines 9 to 10
(memory 1 2 shared)
(data (i32.const -1) "\00")
Copy link
Member

Choose a reason for hiding this comment

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

You can give these names to make the output more readable.

@kripken
Copy link
Member Author

kripken commented Jan 31, 2024

Dynamic linking uses imported offsets, correct. Maybe it's better to try to avoid any regression there, good point.

We already have a flag for "zero memory initialized". If that also let us assume the segments did not trap, that would be enough here, but it is perhaps slightly awkward. (Traps never happen is the more directly relevant flag, but in general C++ can't use it.)

@tlively
Copy link
Member

tlively commented Jan 31, 2024

But I think it would be more complicated to do this idea - we'd need to track split-off parts of the original segment etc

I think this should actually be a fairly localized change to calculateRanges.

@kripken
Copy link
Member Author

kripken commented Feb 1, 2024

Hmm, I don't see how to do that only in calculateRanges? At least that information needs to reach createSplitSegments here:

if (range.isZero) {
continue;
}

That is where we skip zero ranges, but in your suggestion IIUC we'd keep the last zero in the range (to ensure we still trap). So createSplitSegments would need to reason about which zero ranges to keep. We could mark them .neededForTrapping = true perhaps or such, which is not terribly hard to do, but I'm not sure it's worth it.

I checked meanwhile and this doesn't regress any use case I can find:

  • Normal modules have constant offsets and they are all in bounds.
  • Dynamic linking main and side modules have a non-constant offset in the big singleton segment, and we don't optimize that. (We could, actually, with some work, in which case then we'd maybe want to revisit this?)
@tlively
Copy link
Member

tlively commented Feb 1, 2024

It should be sufficient to have calculateRanges merge the last two ranges into a single nonzero range iff the segment has a possibly-trapping offset and the last range is zeroes. That will cause createSplitSegments to keep that data in a segment.

@kripken
Copy link
Member Author

kripken commented Feb 1, 2024

  1. What if the entire segment is zeroes?
  2. This may not help much. I thought the idea was to just keep the last byte (zero or not) of a segment that might trap, but in this case it might keep a lot more?
@tlively
Copy link
Member

tlively commented Feb 1, 2024

Basically calculateRanges just comes up with an alternating sequence of "isZero" ranges that are elided and turned into memory.fill calls as necessary and "!isZero" ranges that are kept. There's no problem if a "!isZero" range actually has zeroes in it since the ranges are the only source of truth for the subsequent splitting. So you could also emit a "!isZero" range for a single zero byte at the end instead of doing the merging like I described above. (It might even work to have a zero-length "!isZero" range; I'm not sure whether the subsequent code will handle that correctly or not.)

@kripken
Copy link
Member Author

kripken commented Feb 1, 2024

Ok, done, see last commits. This is a little more complicated than I like given the amount of benefit, but maybe it's ok, and it is definitely future-proof in that it will help us if we split more segment types later. Note that I had to also add code in another place to handle overflows for this to work.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

LGTM!

@kripken kripken merged commit 845e070 into WebAssembly:main Feb 1, 2024
@kripken kripken deleted the mp.overflow branch February 1, 2024 23:28
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
@gkdn gkdn mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants