Closed Bug 1785776 Opened 3 months ago Closed 3 months ago

Clean up WasmArrayObject and TypedObject logic following bug 1774836

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
106 Branch
Tracking Status
firefox106 --- fixed

People

(Reporter: jseward, Assigned: jseward)

References

(Blocks 2 open bugs)

Details

Attachments

(6 files)

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?

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

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

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

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.

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

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

Pushed by jseward@mozilla.com:
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.
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch
You need to log in before you can comment on or make changes to this bug.