Bug 150131 - Implement Object.values / Object.entries
: Implement Object.values / Object.entries
Status: NEW
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: WebKit Nightly Build
: Unspecified Unspecified
: P2 Normal
Assigned To: Jordan Harband
https://github.com/ljharb/proposal-ob...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-14 11:41 PDT by Jordan Harband
Modified: 2015-11-18 15:19 PST (History)
5 users (show)

See Also:


Attachments
Patch (21.87 KB, patch)
2015-10-14 11:44 PDT, Jordan Harband
darin: review-
darin: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jordan Harband 2015-10-14 11:41:43 PDT
My spec proposal for Object.values and Object.entries is at stage 2. It's about to be implemented in Firefox Nightly.

My intention is to move it to stage 3 next month at TC39 - with a target of stage 4 in January. The requirement for stage 4 is that it be implemented in at least 2 browsers. The sooner this gets into WebKit Nightly, the better for the proposal :-)
Comment 1 Jordan Harband 2015-10-14 11:44:19 PDT
Created attachment 263093 [details]
Patch
Comment 2 Jordan Harband 2015-10-14 11:45:31 PDT
Firefox PR: https://bugzilla.mozilla.org/show_bug.cgi?id=1208464
Comment 3 Darin Adler 2015-10-15 09:46:55 PDT
Comment on attachment 263093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263093&action=review

Generally looks like good work. Worth tuning a little more to use our best practices to give us increased chance for great performance out of the gate.

review- because because the patch doesn’t include a test for Object.entries, and because Object.entries was implemented incorrectly (see comment about the incorrect use of constructEmptyArray).

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:657
> +    Vector<Identifier> propertyStrings;
> +    Vector<Identifier, 16> propertySymbols;

We should have an inline capacity for propertyStrings like the one just below it for propertySymbols. We don’t want to do a heap allocation for the vector’s storage unless there is an unusually large number of strings and symbols.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:663
>          for (size_t i = 0; i < numProperties; i++) {
>              const auto& identifier = properties[i];

Should use a modern for loop:

    for (auto& identifier : properties) {

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:665
> +            propertyStrings.append(identifier);

We can get better performance by using reserveInitialCapacity and uncheckedAppend rather than just doing append, since we know exactly how many properties are going to be added.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:673
>              const auto& identifier = properties[i];

Should use a modern for loop:

    for (auto& identifier : properties) {

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:676
> +                propertySymbols.append(identifier);

We can get better performance by using reserveInitialCapacity and uncheckedAppend rather than just doing append, since we have an upper bound on how many symbols are going to be added.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:684
>          size_t numProperties = properties.size();
>          for (size_t i = 0; i < numProperties; i++) {
>              const auto& identifier = properties[i];

Should use a modern for loop:

    for (auto& identifier : properties) {

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:687
>                      propertySymbols.append(identifier);

We can get better performance by using reserveInitialCapacity and uncheckedAppend rather than just doing append, since we have an upper bound on how many symbols are going to be added.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:689
> +                propertyStrings.append(identifier);

We can get better performance by using reserveInitialCapacity and uncheckedAppend rather than just doing append, since we have an upper bound on how many strings are going to be added.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:696
> +    JSArray* results = constructEmptyArray(exec, 0);

If we can know exactly how many elements we plan to put in the array before creating it, it might be even more efficient to use a length and putDirectIndex rather than the repeated calls to push that this function originally had. It’s not very important to share this array between the three different cases in the switch statement.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:703
> +            results->push(exec, Symbol::create(exec->vm(), static_cast<SymbolImpl&>(*identifier.impl())));

I don’t think static_cast<SymbolImpl&>(*identifier.impl()) is the ideal way to write this. Maybe someone else on the project knows the better approach.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:707
> +        Vector<Identifier> properties;

Should use inline capacity here to avoid heap allocation.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:709
> +            properties.append(identifier);

We can get better performance by using reserveInitialCapacity and uncheckedAppend rather than just doing append, since we know how many properties we are going to add.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:714
> +        for (const auto& identifier : properties) {

I don’t think it’s good to allocate memory and concatenate the two vectors into a single vector just so we can iterate both with a single for loop. Instead we should refactor this so the work is done by a helper function and we can iterate two different vectors. Another, more complex, refactoring option would be to write a wrapper class that makes it possible to iterate two vectors with a for loop without copying the vectors. Either way, we want a version that doesn’t allocate another copy of the two vectors.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:715
> +            PropertyDescriptor desc;

Please don’t use abbreviations in names for new WebKit code. The name “descriptor” would be fine here.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:721
> +            results->push(exec, value);

Do we have a guarantee that push does not raise an exception?

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:726
> +        Vector<Identifier> properties;

Should use inline capacity here to avoid heap allocation.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:728
> +            properties.append(identifier);

We can get better performance by using reserveInitialCapacity and uncheckedAppend rather than just doing append, since we know how many properties we are going to add.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:733
> +        for (const auto& identifier : properties) {

I don’t think it’s good to allocate memory and concatenate the two vectors into a single vector just so we can iterate both with a single for loop. Instead we should refactor this so the work is done by a helper function and we can iterate two different vectors. Another, more complex, refactoring option would be to write a wrapper class that makes it possible to iterate two vectors with a for loop without copying the vectors. Either way, we want a version that doesn’t allocate another copy of the two vectors.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:740
> +            JSArray* entry = constructEmptyArray(exec, 0, 2);

Second argument should be nullptr, not 0.

Do tests cover this code path? I am pretty sure that constructing an array of size 2 and then calling push twice on it will give us an array of length 4. If you look at other code using constructEmptyArray, it uses putDirectIndex to fill in values, not push.

It would be more efficient to use the version of constructArray that takes a pointer to the values and length rather than calling push or putDirectIndex twice. We can simply create the two JSValues first and then call constructArray. Don’t even need a local variable for the array.
Comment 4 Jordan Harband 2015-10-15 11:06:58 PDT
Oops! I have all the Object.entries tests locally but I must have forgotten to `git add` them :-) I'll include them, and also do my best to incorporate your feedback. Thanks!!
Comment 5 Jordan Harband 2015-11-18 15:19:24 PST
This proposal is approved for stage 3 as of yesterday - I'll be resuming the work on this shortly. Thanks again for the thorough review!