6

Implement the Record and Tuple primitives based on objects

 2 years ago
source link: https://bugzilla.mozilla.org/show_bug.cgi?id=1730843
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client
Open Bug 1730843 Opened 3 months ago Updated 2 days ago

Implement the Record and Tuple primitives based on objects

Categories

(Core :: JavaScript Engine, enhancement, P3)

Tracking

(ASSIGNED bug which is in the backlog of work)

ASSIGNED

People

(Reporter: nicolo.ribaudo, Assigned: nicolo.ribaudo)

References

(Blocks 5 open bugs,

URL
)

Details

Attachments

(22 files, 6 obsolete files)

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

Details | Review

+++ This bug was initially created as a clone of Bug #1658309 +++

https://github.com/tc39/proposal-record-tuple

I am opening this bug to track an alternative implementation for Bug #1658309 - "Implement the Record and Tuple proposal". Rather than implementing them as three custom new primitive types, they can be implemented as if they were objects but still be exposed to the JS code as if they were primitives.

This is an alternative implementation to D87232. I didn't implement
object wrappers for these new primitive-like-objects yet, so
typeof Object(rec) is currently "record" and not "object".

They are internally implemented using as dense elements. After that the tuple is fully initialized it's frozen to prevent modifications.

Depends on D125644

This patch adds #{} and #[] support to the parser and to Reflect.parse.
The AST has the same shapre of object and array literals, but the node
names are "RecordExpression" and "TupleExpression".

Since this patch only implements parser support and not bytecode emitter
support, any attempt of evaluating those literals will crash. This is why
I have only used Reflect.parse in the tests.

The ParseNode class sets the hasNonConstInitializerBit flag for records
and tuples, since this will allow us to use a single record/tuple instance
if it's constant but evaluated multiple times.

This patch is effectively the same as D87586

Depends on D125645

This patch introduces three new op codes:

  • InitTuple length preallocates a tuple of the given length: this is the final length if the
    tuple doesn't contain spreads, otherwise it's the most pessimisitc choice possible
    (i.e. spreads are assumed to introduce no new elements). The preallocated tuple size
    can still grow if needed.
  • AddTupleElement pushes the last value in the stack to the tuple (the second-last stack element)
  • FinishTuple marks the tuple as initialized. An unfinished tuple should never leak to JavaScript
    code, and any attempt to get its element will fail.

The bytecode relative to tuple spread is similar to the bytecode used for array spread, emitting
op codes to call the different steps of the iterator protocol.

Depends on D125646

This patch implements the SameValueZero (===) and SameValue (Object.is) algorithms for tuples, and adds the equivalent functions for records (they will crash when called).

Depends on D125647

This patch adds support for properties in records, when created using the Record({}) function.

Record properties are stored as normal object properties, but there is
an additional sortedKeys internal slot containing an array of all the record
keys sorted alphabetically. This will be necessary when comparing two records
for equality, or when enumerating their properties.

Depends on D125649

This patch defines the bytecode for record literals, similarly to how it has been implemented for tuple literals:

  • InitRecord [length] pushes a new record to the stack, allocating
    memory for length properties
  • AddRecordProperty reads the key and the value from the stack,
    and defines it on the record which is being initialized
  • FinishRecord freezes a record and sorts its keys

This patch doesn't implement support for spread in record literals yet.

Depends on D125650

This is done introducing a new opcode, AddRecordSpread, that
takes a value and defines its enumerable properties on the record
which is currently being initialized.

Depends on D125651

This reuses the same implementation strategy used for tuples

Depends on D125652

This patch implements the third new type introduced by the Records and Tuples proposal: Box.

It implements:

  • the "box" primitive type, implemented as an object
  • the Box function and its instance methods
  • boxes equality

Still missing:

  • support for boxes as Map and Set keys
  • Box.containBoxes (this function will likely change in the proposal)

Depends on D125654

This patch fixes Object.keys in records to return sorted keys, and adds
tests for the other Object.* methods.

This patch fixes the missing pieces to make enumeration and object
operations work with tuples (they need to be handled
specially in some places because of the contextual prototype).

Depends on D125656

This patch implements JSON.stringify as specified in the Record and Tuple proposal.

Due to the Box unwrapping logic, balancing between "don't duplicate code" and "keep the R&T code isolated behind the ENABLE_RECORD_TUPLE flag" was tricky: I ended up extracting the code to call toJSON to a separate funcition even
if without the R&T proposal it's used only once.

Depends on D125657

When preparing records, tuples and boxes to be used as a Map/Set key, we
recursively atomize the strings that they contain. This makes it
possible to compute a hash for the whole structure, and to compare them
with an infallible function.

ToDo: Figure out where to store the isAtomized flag for tuples

Depends on D125659

This is in preparation of the next patch which will introduce object wrappers

Depends on D125660

Even if they are still implemented as subclasses of JSObject, representing
them differently in the Value wrapper has a few advantages:

  • Checking the equality semantics of an object (deep vs pointer) is faster,
    since we only need to check the Value's bits.
  • It's easier to differentiate between R/T/B and R/T/B object wrappers,
    since .isObject() returns false for the "primitives". For example,
    this patch fixes the typeof Object(#{}) tests.

This approach's advantages are the same as what we would get implementing them
as primitives from scratch (https://phabricator.services.mozilla.com/D87232),
but it also allows us to re-use the existing objects optimizations.

Depends on D126691

Attachment #9243054 - Attachment description: Bug 1730843 - Part 17 - Add {Record,Tuple,Box}Object wrappers for {Record,Tuple,Box}Type → Bug 1730843 - Part 16 - Add {Record,Tuple,Box}Object wrappers for {Record,Tuple,Box}Type
Assignee: nobody → nicolo.ribaudo

Even if R&T are internally implemented as objects, they are still
primitives and they should be have object wrappers:

typeof Tuple() === "tuple"
typeof Object(Tuple()) === "object"

Depends on D125644

This patch fixes and tests handling of property descriptors for record properties

Depends on D125650

This patch fixes the missing pieces to make object operations work with tuples.

Depends on D129675

Attachment #9241304 - Attachment description: Bug 1730843 - Part 12 - Ensure that tuple object ops work → Bug 1730843 - Part 12 - Add support for iterating tuples
Attachment #9241289 - Attachment description: Bug 1730843 - Part 1 - Add support for empty R&T as "primitive objects" → Bug 1730843 - Part 1 - Add support for empty R&T based on objects
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

The proposal currently specifies R/T/B wrappers as frozen.

This patch makes them look as if they were frozen, while still internally
keeping them marked as extensible. This allows lazily resolving the
wrapper properties (so that we can avoid a full copy every time we
create the wrapper object), but they are exposed as non-extensible to JS
code.

It's still possible that the proposal will switch to make them mutable;
you can follow the discussion (and participate!) at
https://github.com/tc39/proposal-record-tuple/issues/137.

Depends on D125660

Hi Anba,

In this patch, Nicolò has encountered some oddity with make_intl_data.py and the resulting headers. Would you mind taking a peek to give a hint on what's going on; I don't really understand what's going on here.

Flags: needinfo?(andrebargull)

Sorry about those bugs. I've filed bug 1741220 and bug 1741222 to fix this.

Flags: needinfo?(andrebargull)

This is done by not using the NativeGetProperty and NativeDeleteProperty functions for R/T/B, but by creating two new "parallel" functions that handle them specifically.
This is because R/T/B don't behave like classic objects, but they get their prototype lexically (like other primitives).

It's not necessary to reimplement NativeSetProperty, because it always throw since R/T/B are frozen.

Depends on D131124

Since tuples use inline elements, they cannot have internal slots and thus we cannot store the IS_ATOMIZED
flag as we do for records and tuples. However, we can store it in a bit of the ObjectElements flags.

Depends on D132367

The code for converting records and objects to strings would be almost identical:
to avoid dynamic branches I converted them to if constexpr statements.

I also found that we didn't have a test for boxed R&T (only for boxes Box).

Ref https://phabricator.services.mozilla.com/D132367#inline-730448

Depends on D132367

This patch fixes a regression introduced by the --enable-record-tuple build flag, which made
https://searchfox.org/mozilla-central/source/js/src/tests/non262/template-strings/bug1559123.js
crash because the IsExtendedPrimitive doesn't work with forwarded objects.

This patch introduces a new gc::MaybeForwardedIsExtendedPrimitive function, similar to
the existing gc::MaybeForwardedObjectIs one.

Depends on D132393

Attachment #9253022 - Attachment description: Bug 1730843 - Part 19 - Run Records&Tuples tests on ci → Bug 1730843 - Part 20 - Run Records&Tuples tests on ci

Comment on attachment 9253022 [details]
Bug 1730843 - Part 20 - Run Records&Tuples tests on ci

Revision D132467 was moved to bug 1744967. Setting attachment 9253022 [details] to obsolete.

Attachment #9253022 - Attachment is obsolete: true
Summary: Implement the Record and Tuple proposal based on objects → Implement the Record and Tuple primitives based on objects

After the feedback received at the Dec 2021 TC39 meeting, it's very unlikely
that the proposal will include Box. This patch removes it so that the
rest of the R&T implementation can be merged.

Attachment #9255723 - Attachment description: WIP: Bug 1730843 - Part 20 - Remove Box from the Record&Tuple implementation → Bug 1730843 - Part 20 - Remove Box from the Record&Tuple implementation
Attachment #9250695 - Attachment description: Bug 1730843 - Part 16 - Mark R/T/B object wrappers as frozen → Bug 1730843 - Part 16 - Mark R&T object wrappers as frozen
You need to log in before you can comment on or make changes to this bug.

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK