Bug 158530 - We should be able to lookup symbols by identifier in builtins
Summary: We should be able to lookup symbols by identifier in builtins
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-08 10:10 PDT by Keith Miller
Modified: 2016-06-08 12:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (18.02 KB, patch)
2016-06-08 10:48 PDT, Keith Miller
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-06-08 10:10:22 PDT
We should be able to lookup symbols by identifier in builtins
Comment 1 Keith Miller 2016-06-08 10:48:04 PDT
Created attachment 280812 [details]
Patch
Comment 2 Keith Miller 2016-06-08 10:53:09 PDT
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.
Comment 3 Mark Lam 2016-06-08 11:38:39 PDT
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).
Comment 4 Keith Miller 2016-06-08 12:39:07 PDT
Committed r201825: <http://trac.webkit.org/changeset/201825>