| Summary: | Implement Object.values / Object.entries | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jordan Harband <ljharb> | ||||
| Component: | JavaScriptCore | Assignee: | Jordan Harband <ljharb> | ||||
| Status: | RESOLVED DUPLICATE | ||||||
| Severity: | Normal | CC: | darin, fpizlo, ggaren, gskachkov, jfbastien, msaboff, saam, ticaiolima, ysuzuki | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| URL: | https://github.com/ljharb/proposal-object-values-entries | ||||||
| Attachments: |
|
||||||
|
Description
Jordan Harband
2015-10-14 11:41:43 PDT
Created attachment 263093 [details]
Patch
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. 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!! 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! Jordan: I was wondering if you'd had time to look into this? I was just about to use it and realized we didn't support it yet. No, I'm afraid I haven't, and won't for the medium term. The patch attached to this bug is the latest I've got; I'd be delighted if someone took it and ran with it. (In reply to comment #6) > Jordan: I was wondering if you'd had time to look into this? I was just > about to use it and realized we didn't support it yet. Hmm, sound weird. It was implemented in following patch https://bugs.webkit.org/show_bug.cgi?id=160410 Could you please provide script that we do no support? I'll take a look (In reply to comment #8) > (In reply to comment #6) > > Jordan: I was wondering if you'd had time to look into this? I was just > > about to use it and realized we didn't support it yet. > > Hmm, sound weird. It was implemented in following patch > https://bugs.webkit.org/show_bug.cgi?id=160410 > Could you please provide script that we do no support? I'll take a look Oh ignore me! I was going on MDN's browser support table: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries It links to this bug, not the new one, and I figured bugs don't lie (they do). I'll close this one as dupe, thanks! *** This bug has been marked as a duplicate of bug 160410 *** (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #6) > > > Jordan: I was wondering if you'd had time to look into this? I was just > > > about to use it and realized we didn't support it yet. > > > > Hmm, sound weird. It was implemented in following patch > > https://bugs.webkit.org/show_bug.cgi?id=160410 > > Could you please provide script that we do no support? I'll take a look > > Oh ignore me! I was going on MDN's browser support table: > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/ > Global_Objects/Object/entries > > It links to this bug, not the new one, and I figured bugs don't lie (they > do). > > I'll close this one as dupe, thanks! > > *** This bug has been marked as a duplicate of bug 160410 *** Ohh, ok I'll try fix documentation on MDN. To check implementation of statuses ES6/ES7 features I prefer to use kangax table http://kangax.github.io/compat-table/es2016plus/#test-Object_static_methods_Object.values. |