1785776 - Clean up WasmArrayObject and TypedObject logic following bug 1774836

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Julian Seward [:jseward]]

Bug 1774836 creates separate representations for wasm arrays and structs,
thereby facilitating further cleanups of WasmArrayObject and the logic
associated with the common parent class TypedObject. These are:

• WasmArrayObject: move the array length field back into the the object
  proper, so that:

  • the array contents vector is now guaranteed to be at least 8-aligned;
    currently we are only ensured a 4-alignment, due to presence of the length
    field at the start

  • reads/writes of the array have to visit only 2 cache lines (probably)
    rather than 3 (probably). At present the sequence is:

    • visit object near start, to get pointer to OOL block
    • visit start of OOL block, to get the length field
    • visit the data itself

    Nothing ensures that any of those addresses are close to each other.
    Putting the length field back in the object itself makes it likely to live
    in the same line as the OOL-block-pointer.

• logic associated with TypedObject: code relating to WasmStructObject and
  WasmArrayObject has a rather hand-rolled-RTTI style, with considerable use
  of is<..>() and as<..>(). This could be reduced in scope.

  For example, Instance::structNew calls TypedObject::createStruct, which
  creates a WasmStructObject, but then passes it to initDefault, which
  accepts a TypedObject, which immediately checks the object's tag and
  down-casts back to WasmStructObject to initialise it.

• also, perhaps, rename TypedObject to a name that's more obviously related to
  its children, Wasm{Struct,Array}Object. Maybe WasmAggregateObject or
  WasmObject?

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1785776-1-cleanup-WAO.diff-2

Patch for safekeeping, until it comes time to polish/review:
First bullet point in comment 0: changes WasmArrayObject representation

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1785776-2-cleanup-generally.diff-1

Second bullet point in comment 0: remove some home-made RTTIery
and delete various other unused methods. Also rename some variables.

Comment 3

[Julian Seward [:jseward]]

Attachment: bug1785776-3-resequence-rename.diff-1

Third bullet point in comment 0: resequence the .cpp file to match
the .h file. And rename TypedObject* to WasmGcObject*.

Comment 4

[Julian Seward [:jseward]]

Attachment: Bug 1785776 - Clean up WasmArrayObject and TypedObject logic following bug 1774836 (Part 1). r=rhunt.

Bug 1774836 creates separate representations for wasm arrays and structs,
thereby facilitating further cleanups of WasmArrayObject and the logic
associated with the common parent class TypedObject.

This patch modifies WasmArrayObject. It moves the array length field back
into the the object proper, so that:

• the array contents vector is now guaranteed to be at least 8-aligned;
  currently we are only ensured a 4-alignment, due to presence of the length
  field at the start

• reads/writes of the array have to visit only 2 cache lines (probably) rather
  than 3 (probably). At present the sequence is:

  • visit object near start, to get pointer to OOL block
  • visit start of OOL block, to get the length field
  • visit the data itself
    Nothing ensures that any of those addresses are close to each other.
    Putting the length field back in the object itself makes it likely to live
    in the same line as the OOL-block-pointer.

Specific changes:

• WasmArrayObject

  • new field numElements_; accessors updated accordingly
  • method addressOfElementZero() removed
  • method create() removed; it has been inlined into its one-and-only caller.
  • method outOfLineTypedMem() renamed to data()

• RttValue::Offset

  • the value meaning "array length" has been changed from 0 to 2^32-1.
  • Values indicating array offsets have had the bias-value of 4 removed.
  • Values 1, 2 and 3 now have a meaning when they didn't before.
  • Producer (RttValue::lookupProperty) and consumer (TypedObject::loadValue)
    are updated accordingly.

• TypedObject::loadValue ridealong fix: I suspect the previous implementation
  had always been buggy in the sense that it used the array element type
  (type) also when fetching the length field (by calling ToJSValue). It now
  passes ValType::I32 in that case.

• BaseCompiler::emitGcArrayAdjustDataPointer() removed - no longer needed

• corresponding baseline compiler updates (simplifications)

• In a few places, variables have been renamed accordingly.

Comment 5

[Julian Seward [:jseward]]

Attachment: Bug 1785776 - Clean up WasmArrayObject and TypedObject logic following bug 1774836 (Part 2). r=rhunt.

Previous patches in bug 1774836 and in this bug changed the representations of
wasm typed arrays and structs. This patch cleans up the overall resulting
code. In particular the goal is to simplify cases where an object is created,
and we know whether it is a WasmArrayObject or a WasmStructObject, then it is
up-cast to a TypedObject, and then manually RTTI'd so as to perform some
object-specific action. Changes:

• TypedObject::createStruct replaced by WasmStructObject::createStruct

• TypedObject::createArray replaced by WasmArrayObject::createArray

• Renamed some structObj to wso and arrayObj to wao in keeping with
  the style of previous patches.

• TypedObject::initDefault removed; relevant parts have been inlined into its
  callers (the ::create* methods)

• TypedObject::visitReferences removed; relevant parts have been inlined into
  its callers (the ::obj_trace methods)

• TypedObject::offsetOfRttValue removed; no longer necessary.

• TypedObject is no longer a friend class of Wasm{Array,Struct}Object; it has
  been un-friended.

• WasmArrayObject::numElements_ has been made non-private for convenience and
  for consistency with the ::data_ field.

Depends on D156043

Comment 6

[Julian Seward [:jseward]]

Attachment: Bug 1785776 - Clean up WasmArrayObject and TypedObject logic following bug 1774836 (Part 3). r=rhunt.

Some non-functional, renaming-style cleanups:

• TypedObject has been consistently renamed to WasmGcObject, both as a class
  name and as file names.

• WasmGcObject.h (new name):

  • removed unused IsWasm{Struct,Array}ObjectClass()
  • removed unused JSObject::is<js::Wasm{Struct,Array}Object>()

• WasmGcObject.cpp (new name) has been re-sequenced to match its header file.

• TypedObject-inl.h has been removed. Its one-and-only method
  ::allocKindForRttValue() has been moved back to the "standard" header
  (WasmGcObject.h). The only use point for that method is from an
  expensive-looking method (caller) so there's no point in inlining it. Can
  be reinstated later if profiling shows it is hot.

Depends on D156044

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d736d1c60644
Clean up WasmArrayObject and TypedObject logic following bug 1774836 (Part 1).  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/7a85b7715658
Clean up WasmArrayObject and TypedObject logic following bug 1774836 (Part 2).  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/5d3eee06ffdd
Clean up WasmArrayObject and TypedObject logic following bug 1774836 (Part 3).  r=rhunt.

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/d736d1c60644
https://hg.mozilla.org/mozilla-central/rev/7a85b7715658
https://hg.mozilla.org/mozilla-central/rev/5d3eee06ffdd