Bug 158530 – We should be able to lookup symbols by identifier in builtins

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

We should be able to lookup symbols by identifier in builtins

[Keith Miller]

Created attachment 280812 [details]
Patch

[Keith Miller]

Comment on attachment 280812 [details]
Patch

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

> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:82
>          let wrapper = {
> -            [@symbolIterator]() {
> +            @iteratorSymbol() {
>                  return iterator;
>              }

Instead of using an object literal we could change this and others to 

let wrapper = {}
wrapper.@iteratorSymbol = function() { return iterator; };

This change would mean we wouldn't need the emitLoad change. I'm not sure if that's worth it though since it might be hard, in the future, to understand why a literal with a symbol does not work.

[Mark Lam]

Comment on attachment 280812 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        This patch allows us to lookup the value of an symbol property on a object

typo: /an/a/

> Source/JavaScriptCore/ChangeLog:13
> +        Symbol.iterator. Even though as we tier up we will convert the builtin
> +        by value symbol lookups into identifier lookups there is still a significant
> +        performance difference between get_by_id and get_by_val in the LLInt.

This last sentence "Even though ... the LLInt." is confusing to me.  Can you break it up or add some relevant commas?  How about something like:
"As we tier up, we will convert the builtin "by value" symbol lookups into fast identifier lookups.  However, there is still a significant perf...".  Did I understand you right?

>> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:82
>>              }
> 
> Instead of using an object literal we could change this and others to 
> 
> let wrapper = {}
> wrapper.@iteratorSymbol = function() { return iterator; };
> 
> This change would mean we wouldn't need the emitLoad change. I'm not sure if that's worth it though since it might be hard, in the future, to understand why a literal with a symbol does not work.

Per our offline discussion, you told me that it is more performant to use the 2nd (not object literal) form.  Let's go ahead and use that form instead, and remove the emitLoad() hack.  Please also add a comment to warn future code authors from changing this into an object literal (because it won't work).

[Keith Miller]

Committed r201825: <http://trac.webkit.org/changeset/201825>