Closed Bug 1669784 Opened 2 months ago Closed 2 months ago

Remove TypedObject code not required for GC-MVP

Categories

(Core :: Javascript: WebAssembly, task, P3)

task

Tracking

()

RESOLVED FIXED

People

(Reporter: rhunt, Assigned: rhunt)

References

(Blocks 1 open bug)

Details

(Keywords: perf-alert)

Attachments

(14 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

We want to use the TypedObject API for the basis of our GC-MVP JS-API, but there's a bunch of features we don't need or will need to change. This bug is for cutting all features not required by our current GC prototype. This will make it much easier to turn it into what we need going forward.

The major changes I have so far:

  1. Remove all notions of opaque vs. transparent. Most of this code was already gone.
  2. Remove all helper methods from prototype chains (.array, .equivalent, .forEach, .toSource)
  3. Simplify prototype chain so all StructType/ArrayType's don't share a prototype
  4. Disallow directly nested struct/arrays without indirection
  5. Disallow setting fields/elements from JS
  6. Move implementation of getting fields/elements to native code, removing last need for self-hosted code
  7. Disallow creating StructType/ArrayType from JS
  8. Remove TypedObject namespace disallowing direct access to .uint8, .int8, .object, .StructType, .ArrayType, etc.
  9. Remove support for types not needed by Wasm, such as string, uint32, uint64
  10. Remove JS_HAS_TYPED_OBJECTS feature code and use Wasm GC feature code
  11. Remove all typed object API tests, update Wasm GC tests for slight incompatibilities

I think this is a good starting point, so I'll post the patches here soon.

We want all TypedObjects to be opaque so kill all code related to transparent
TypedObjects.

We may want some of these in the future, but for now they're a maintenance burden.

Depends on D92849

A future extension to the GC proposal may allow flattened data structures and
interior pointers, but for now let's remove support for this by disallowing it
at the type level.

The reading/writing code will be rewritten/removed in a later commit so no
significant simplifications are made there.

Depends on D92850

For now it's sufficient to only support mutating these objects from
Wasm. This will make it so we only need to support type checks on entry
to Wasm functions for now.

Depends on D92851

This will allow us to only worry about creating these types from WebAssembly.

Depends on D92853

The code for reading fields/elements is the only remaining self-hosted code. Replace it with
a much smaller native implementation.

Depends on D92854

Follow-up work is going to integrate this code more extensively with WebAssembly's
type system and runtime, so I think it makes sense for the implementation to
live in wasm/.

Depends on D92855

This commit merges the TypedObject namespace into the WebAssembly namespace, and
only exposes the TypedObject definitions when the GC feature is enabled.

A future commit will remove the TypedObject definitions from the namespace, but we
still need the infrastructure from this commit for storing the TypedObject definitions
in private slots in the namespace.

Depends on D92856

There's no point in exposing the TypedObject definitions in the WebAssembly namespace
until we allow the types to be constructed by JS again.

This commit also removes support for TypedObject types that aren't needed by
WebAssembly, such as 'string', 'uint32', 'uint64'.

Depends on D92857

Fix tests for:

  • disallowing construction of TypedObject types
  • disallowing JS to set fields of struct types

Depends on D92858

This commit removes all tests that used the old TypedObject API.

We may be able to salvage some of these tests in the future when
the Wasm GC API is clearer, but for now remove them.

Depends on D92859

Blocks: 1649247
Blocks: wasm-gc-mvp
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/89e5f4b08331
Remove opaque/transparent split from TypedObjects. r=jandem
https://hg.mozilla.org/integration/autoland/rev/792089710c63
Remove helper methods from TypedObjects. r=jandem
https://hg.mozilla.org/integration/autoland/rev/62ad099fe894
Disallow nested TypedObjects without an indirection. r=jandem
https://hg.mozilla.org/integration/autoland/rev/18cdaa6614cf
Disallow setting fields/elements of TypedObjects from JS. r=jandem
https://hg.mozilla.org/integration/autoland/rev/248d39d51318
Don't share prototypes between all StructType/ArrayTypes. r=jandem
https://hg.mozilla.org/integration/autoland/rev/331a512fee82
Don't allow constructing a StructType/ArrayType from JS. r=jandem
https://hg.mozilla.org/integration/autoland/rev/506ef415351b
Replace self-hosted TypedObject read field/element code with native implementation. r=jandem
https://hg.mozilla.org/integration/autoland/rev/aa2da8a6cc39
Move TypedObject implementation from builtin/ to wasm/. r=lth
https://hg.mozilla.org/integration/autoland/rev/481b2e98e24b
Merge TypedObject namespace into WebAssembly namespace, gated on GC feature. r=lth
https://hg.mozilla.org/integration/autoland/rev/01f901ce054b
Don't expose TypedObject definitions on WebAssembly namespace. r=jandem
https://hg.mozilla.org/integration/autoland/rev/59d7cd2a604c
Fix wasm/gc tests for changes to TypedObject API. r=lth

I was not able to push the last commit, removing all typed object tests, as lando was giving me an error. My guess is that it was too big.

I'm going to try to land it separately. This is okay, because all TypedObject tests do feature detection and so they should not run when the TypedObject namespace is no longer available.

Keywords: leave-open

Ah, some browser tests are asserting that TypedObjects are defined in nightly.

Flags: needinfo?(rhunt)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/69cd2f4baa75
Remove opaque/transparent split from TypedObjects. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d075286992c2
Remove helper methods from TypedObjects. r=jandem
https://hg.mozilla.org/integration/autoland/rev/476165c19fa2
Disallow nested TypedObjects without an indirection. r=jandem
https://hg.mozilla.org/integration/autoland/rev/15e9d163baba
Disallow setting fields/elements of TypedObjects from JS. r=jandem
https://hg.mozilla.org/integration/autoland/rev/34d573a984fc
Don't share prototypes between all StructType/ArrayTypes. r=jandem
https://hg.mozilla.org/integration/autoland/rev/921a5bd4b0c5
Don't allow constructing a StructType/ArrayType from JS. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ac1e917a452b
Replace self-hosted TypedObject read field/element code with native implementation. r=jandem
https://hg.mozilla.org/integration/autoland/rev/970f38ba9b25
Move TypedObject implementation from builtin/ to wasm/. r=lth
https://hg.mozilla.org/integration/autoland/rev/42ef5b507f58
Merge TypedObject namespace into WebAssembly namespace, gated on GC feature. r=lth
https://hg.mozilla.org/integration/autoland/rev/6f7124a9d2f2
Don't expose TypedObject definitions on WebAssembly namespace. r=jandem
https://hg.mozilla.org/integration/autoland/rev/601765c9159a
Fix wasm/gc tests for changes to TypedObject API. r=lth

Depends on D93520

Attachment #9180318 - Attachment description: Bug 1669784 - Remove all TypedObject tests. r?jandem → Bug 1669784 - Remove TypedObject tests, part 3. r?jandem
Attachment #9181578 - Attachment description: Bug 1669784 - Remove TypedObject tests, part 2. → Bug 1669784 - Remove TypedObject tests, split part 2.
Attachment #9180318 - Attachment description: Bug 1669784 - Remove TypedObject tests, part 3. r?jandem → Bug 1669784 - Remove TypedObject tests, split part 3. r?jandem
Attachment #9181577 - Attachment description: Bug 1669784 - Remove TypedObject tests, part 1. → Bug 1669784 - Remove TypedObject tests, split part 1.
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/687ca86e0d13
Remove TypedObject tests, split part 1.
https://hg.mozilla.org/integration/autoland/rev/ac3e2a0fb832
Remove TypedObject tests, split part 2.
https://hg.mozilla.org/integration/autoland/rev/3a9738afa9d0
Remove TypedObject tests, split part 3. r=jandem
Status: NEW → RESOLVED
Closed: 2 months ago
Keywords: leave-open
Resolution: --- → FIXED

== Change summary for alert #27244 (as of Fri, 16 Oct 2020 07:28:25 GMT) ==

Improvements:

Ratio Suite Test Platform Options Absolute values (old vs new)
0.36% Base Content JS windows7-32-shippable 2,277,687.00 -> 2,269,477.33
0.32% Base Content JS windows7-32-shippable 2,276,536.00 -> 2,269,138.67
0.31% Base Content JS linux1804-64-shippable 2,895,450.29 -> 2,886,586.67
0.30% Base Content JS windows10-64-shippable-qr 2,900,150.29 -> 2,891,510.00
0.29% Base Content JS linux1804-64-shippable-qr 2,895,199.43 -> 2,886,745.33
0.28% Base Content JS windows10-64-shippable 2,890,893.71 -> 2,882,934.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=27244

Keywords: perf-alert
You need to log in before you can comment on or make changes to this bug.