Avoid newly exposing prototypes in GTO by tlively · Pull Request #9043 · WebAssembly/binaryen

tlively · 2026-08-26T01:34:08Z

When GTO removes fields or makes them immutable, it may introduce an immutable externref first field on the descriptor of a JS-exposed type where there was none before. That means that the described type could now have a JS-observable prototype where it did not before optimization, which makes this a misoptimization.

Fix the problem by inserting an i8 placeholder first field wherever we would otherwise start exposing a prototype where there was none before. This is expected to be exceptionally rare in practice, so the extra memory use is not expected to be a real problem.

Instead of adding a placeholder field, we could have inhibited optimization of the existing first field, but that would be more likely than an unaccessed placeholder field to have adverse effects in later passes.

Fixes #9026.

When GTO removes fields or makes them immutable, it may introduce an immutable externref first field on the descriptor of a JS-exposed type where there was none before. That means that the described type could now have a JS-observable prototype where it did not before optimization, which makes this a misoptimization. Fix the problem by inserting an i8 placeholder first field wherever we would otherwise start exposing a prototype where there was none before. This is expected to be exceptionally rare in practice, so the extra memory use is not expected to be a real problem. Instead of adding a placeholder field, we could have inhibited optimization of the existing first field, but that would be more likely than an unaccessed placeholder field to have adverse effects in later passes. Fixes #9026.

kripken · 2026-08-27T18:32:37Z

+    ;; CHECK-NEXT:  (type $struct (descriptor $desc) (struct))
+    (type $struct (descriptor $desc) (struct))
+    ;; CHECK:       (type $desc (describes $struct) (struct (field i8) (field externref)))
+    (type $desc (describes $struct) (struct (field (mut externref))))


It's not clear to me why you say this would be "exceptionally rare" in practice? Because externrefs are rare? Otherwise, a mutable one seems common, and in the first slot also seems common.

Externrefs to represent strings and other JS content may be pretty common in some situations?

Because descriptors in practice are vtables, which I don't think are likely to contain strings or other externrefs. Maybe some toolchain would put a class name string in the vtable?

I see, thanks, that's what I was missing. So this is just on descriptors, not general structs.

kripken

Also, I am uneasy with this tradeoff. The benefits of immutability are large in general, but less so for an externref field (there is no constant we can propagate). And adding an i8 is a definite cost. I would actually lean towards the opposite tradeoff?

tlively · 2026-08-27T23:26:26Z

That's for the case of a mutable externref field being made immutable, but what about the case where an externref field is shifted into the first position?

kripken · 2026-08-28T00:00:26Z

We could also avoid shifting it there? Basically, changes that turn it into a prototype would be disallowed. You're right that I didn't consider shifting, so maybe this is more common/complex than I thought, but adding a field seems like a "heavy" operation...

tlively · 2026-08-28T00:11:36Z

We could avoid shifting, but the effect would be at least as bad as the placeholder i8 because that would mean keeping whatever prefix of fields we would have removed that would have caused the externref to shift into the first position.

I'm pretty happy with the placeholder field trade off because it doesn't inhibit any other optimizations we would do and works no matter what the invalid transformation would have been. Since we only add placeholders to descriptors, which are much rarer than normal objects at runtime, the memory overhead of the placeholder fields is expected to be tiny, even if we do end up needing lots of placeholders for some reason.

kripken

Ok, fair points on the tradeoff, sg

kripken · 2026-08-28T15:52:25Z

+    bool hasPlaceholder = false;
+
+    IndexAnalysis(const std::vector<Index>& indexes) {
+      Index maxIndex = 0;


Perhaps add some comments to this class and/or properties? Just reading the code, I'd expect newSize to mean the size after removals - is that right? And the input is the mapping of old indexes to new indexes?

Yes, newSize is the size after removals and possible placeholder insertion. And yes, the input is the old to new mapping. I'll add comments.

kripken · 2026-08-28T15:53:53Z

+      if (!removableIndexes.empty() || superHasUpdates || isExposedNoProto) {
+        // We might be removing fields. Reorder them to allow that, as in the
+        // general case we can only remove fields from the end, so that if our
+        // subtypes still need the fields they can append them. For example:


Please comment about the importance of isExposedNoProto to the placeholder issue

kripken · 2026-08-28T15:56:20Z

+            if (auto it = canBecomeImmutable.find(type);
+                it != canBecomeImmutable.end() && i < it->second.size() &&
+                it->second[i]) {
+              optimizedField.mutable_ = Immutable;


Why do we apply immutability here? That is, why does this duplicate the normal code that turns fields immutable?

We need to check if the normal optimization would produce an immutable externref in the first field without our placeholder intervention. If we didn't check what normal optimization would do with immutability here, then we would either end up adding unnecessary placeholders when the first field would become a mutable externref or end up missing necessary placeholders when the first field is optimized from mutable to immutable.

Can't we do this in that normal code, then?

I mean that it seems odd to have two places in the code that turn things immutable.

kripken · 2026-08-28T15:57:01Z

+              for (auto& idx : indexesAfterRemoval) {
+                if (idx != RemovedField) {
+                  ++idx;
+                }


Please add a comment that we do not add the i8 here, we just make room for it, and that room - the missing index 0 - is the marker we use to identify the need to add the i8 later (is that right?)

kripken · 2026-08-28T15:58:22Z

+
+    // Also propagate exposed descriptors to supertypes so that descriptor
+    // hierarchies have consistent layouts. Do not propagate to supertypes that
+    // actually expose a prototype.


Why didn't we need this code before?

Why do we not propagate as per the second line here?

We didn't need this code before because exposedNoProtoDescs is a new set that is used just to help determine when to add placeholders. I guess the comment should say "propagate lack of exposed descriptors."

The case where we stop propagating is the edge case where a subtype has nullexternref and the supertype has externref as its immutable first field. We do not want to add a placeholder to the supertype in that case because it actually does expose a prototype. I'll add a comment about that to the code.

kripken · 2026-08-28T15:59:04Z

+        }
+      }
+      newSize = hasKept ? maxIndex + 1 : 0;
+      hasPlaceholder = hasKept && !hasIndexZero;


This seems a bit "magical" - worth a comment

kripken · 2026-08-28T16:01:00Z

-          // we have done pointless work).
+        operands.resize(analysis.newSize);
+        if (analysis.hasPlaceholder) {
+          operands[0] = Builder(*getModule()).makeConst(Literal(int32_t(0)));


Suggested change

operands[0] = Builder(*getModule()).makeConst(Literal(int32_t(0)));

// What we put in the i8 placeholder does not matter.

operands[0] = Builder(*getModule()).makeConst(Literal(int32_t(0)));

tlively requested a review from a team as a code owner August 26, 2026 01:34

tlively requested review from aheejin and removed request for a team August 26, 2026 01:34

tlively force-pushed the fix-9026 branch from cc68dc5 to 1f67016 Compare August 27, 2026 01:18

tlively requested a review from kripken August 27, 2026 17:53

kripken reviewed Aug 27, 2026

View reviewed changes

kripken reviewed Aug 28, 2026

View reviewed changes

	operands[0] = Builder(*getModule()).makeConst(Literal(int32_t(0)));
	// What we put in the i8 placeholder does not matter.
	operands[0] = Builder(*getModule()).makeConst(Literal(int32_t(0)));

Uh oh!

Conversation

tlively commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

tlively commented Aug 27, 2026

Uh oh!

kripken commented Aug 28, 2026

Uh oh!

tlively commented Aug 28, 2026

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tlively commented Aug 26, 2026 •

edited

Loading