Bug 157080 – Change ProxyObject.[[Get]] not to use custom accessor

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[André Bargull]

SVN: rev200124
Build with: perl Tools/Scripts/build-jsc --gtk --debug


The following test case triggers this assertion error:
---
ASSERTION FAILED: from.isCell() && from.asCell()->JSCell::inherits(std::remove_pointer<To>::type::info())
---


Test case:
---
Reflect.get(new Proxy({}, {}), 0, 1)
---


Stack trace:
---
#0  0x00007ffff6e289ac in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:322
#1  0x00007ffff6e289bd in WTFCrashWithSecurityImplication () at ../../Source/WTF/wtf/Assertions.cpp:342
#2  0x00007ffff5fe50be in JSC::jsCast<JSC::JSObject*> (from=...) at ../../Source/JavaScriptCore/runtime/JSCell.h:244
#3  0x00007ffff6cf21c8 in JSC::performProxyGet (exec=0x7fffffffcae0, thisValue=-281474976710655, propertyName=...) at ../../Source/JavaScriptCore/runtime/ProxyObject.cpp:103
#4  0x00007ffff6cebb4e in JSC::PropertySlot::customGetter (this=0x7fffffffca80, exec=0x7fffffffcae0, propertyName=...) at ../../Source/JavaScriptCore/runtime/PropertySlot.cpp:39
#5  0x00000000004423b2 in JSC::PropertySlot::getValue (this=0x7fffffffca80, exec=0x7fffffffcae0, propertyName=...) at ../../Source/JavaScriptCore/runtime/PropertySlot.h:294
#6  0x0000000000448cfb in JSC::JSValue::get (this=0x7fffffffca40, exec=0x7fffffffcae0, propertyName=..., slot=...) at ../../Source/JavaScriptCore/runtime/JSCJSValueInlines.h:768
#7  0x00007ffff6cfc288 in JSC::reflectObjectGet (exec=0x7fffffffcae0) at ../../Source/JavaScriptCore/runtime/ReflectObject.cpp:181
#8  0x00007fffb0bff028 in ?? ()
#9  0x00007fffffffcb60 in ?? ()
#10 0x00007ffff6a33818 in llint_entry () from /home/andre/svn/webkit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18
Backtrace stopped: frame did not save the PC
---

[Yusuke Suzuki]

Created attachment 278946 [details]
Patch

[Yusuke Suzuki]

Created attachment 278948 [details]
Patch

[Yusuke Suzuki]

Created attachment 278951 [details]
Patch

[Saam Barati]

Comment on attachment 278951 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/PropertySlot.h:97
> +    typedef EncodedJSValue (*GetValueFunc)(ExecState*, EncodedJSValue thisValue, PropertyName, JSObject* slotBase);

Can you add a comment here talking about the difference between a CustomAccessorGetter and a CustomValueGatter, and what the arguments are for them?
And then maybe a fixme to see if we can combine them? I think we can since you're adding this new argument. But it requires a lot of one line fixes.

[Yusuke Suzuki]

Comment on attachment 278951 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/PropertySlot.h:97
>> +    typedef EncodedJSValue (*GetValueFunc)(ExecState*, EncodedJSValue thisValue, PropertyName, JSObject* slotBase);
> 
> Can you add a comment here talking about the difference between a CustomAccessorGetter and a CustomValueGatter, and what the arguments are for them?
> And then maybe a fixme to see if we can combine them? I think we can since you're adding this new argument. But it requires a lot of one line fixes.

Nice! Added.

[Yusuke Suzuki]

Committed r201322: <http://trac.webkit.org/changeset/201322>

[Yusuke Suzuki]

*** Bug 154320 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Committed r201331: <http://trac.webkit.org/changeset/201331>

[Gavin Barraclough]

Yusuke,

I'm not keen on this change - this actually reverse a change I made a couple of months ago (removing slotBase from getters): https://bugs.webkit.org/show_bug.cgi?id=154009

Previously having both the thisValue and slotBase passed into getters resulted in them being used inconsistently and incorrectly (and is clearly inefficient if they are not both required).

And they never were both required:
- In the case of custom value getters, you should only ever be operating on the holder of the property - the slotBase.
- In the case of custom accessors you should only ever be operating on the receiver of the access - the thisValue.

I have two specific concerns with this change:

1) In JSBoundSlotBaseFunction you are using slotBase. All use of slotBase in JSBoundSlotBaseFunction is incorrect. We use JSBoundSlotBaseFunction  to reflect custom accessor as JSFunctions, but custom accessor SHOULD NOT be binding to the object they are extracted from. This is not correct behavior. If you follow through the logic you'll find that the value of slotBase  is never actually used prior to your change (as it should not be). I was just working on a patch to rename this class & remove m_slotBase [ https://bugs.webkit.org/show_bug.cgi?id=157978 ]. Reintroducing this use of slotBase is almost certainly a bug.

2) Your patch does not change the value being passed in as the thisValue. This means that in the case of custom value getters, you've changed it such that we're passing the slotBase into the getter twice, rather than passing thisValue & slotBase. If you are going to pass two base values in to the getter (which, again, I don't think we should - it's unnecessary, inefficient, and error-prone), then you should make the If we are going to pass the slotBase in to getters, then I think we should make the thisValue actually be the thisValue, and not a second copy of slotBase.

I must admit I don't yet understand why this change was necessary as a part of a fix to this bug – I'll take a closer look at the test case.

[Gavin Barraclough]

Hmmm, from a high level description of the operation it sounds like Reflect.get should should just be getting the property descriptor from the target, if it's a value return it, if it's an accessor call it passing receiver. But maybe that doesn't match the spec - I'll have to dig into that.

[Yusuke Suzuki]

(In reply to comment #9)
> Yusuke,

Gavin, thank you for your notification :)

> 
> I'm not keen on this change - this actually reverse a change I made a couple
> of months ago (removing slotBase from getters):
> https://bugs.webkit.org/show_bug.cgi?id=154009
> 
> Previously having both the thisValue and slotBase passed into getters
> resulted in them being used inconsistently and incorrectly (and is clearly
> inefficient if they are not both required).
> 
> And they never were both required:
> - In the case of custom value getters, you should only ever be operating on
> the holder of the property - the slotBase.
> - In the case of custom accessors you should only ever be operating on the
> receiver of the access - the thisValue.

Hmm, this change was made because ProxyObject's performProxyGet require both, slotBase and receiver.
When the custom getter is called, performProxyGet need to extract ProxyObject from slotBase, and
later, when the proxy traps, it invokes the handler with receiver.

Now, I'm attempting to discuss with Saam about performProxyGet on IRC (Maybe, now PST is super late, right?). This is only the function that requires the both currently.
And if we can remove this custom accessor usage, we can clean up.

> 
> I have two specific concerns with this change:
> 
> 1) In JSBoundSlotBaseFunction you are using slotBase. All use of slotBase in
> JSBoundSlotBaseFunction is incorrect. We use JSBoundSlotBaseFunction  to
> reflect custom accessor as JSFunctions, but custom accessor SHOULD NOT be
> binding to the object they are extracted from. This is not correct behavior.
> If you follow through the logic you'll find that the value of slotBase  is
> never actually used prior to your change (as it should not be). I was just
> working on a patch to rename this class & remove m_slotBase [
> https://bugs.webkit.org/show_bug.cgi?id=157978 ]. Reintroducing this use of
> slotBase is almost certainly a bug.

Sounds reasonable.

> 
> 2) Your patch does not change the value being passed in as the thisValue.
> This means that in the case of custom value getters, you've changed it such
> that we're passing the slotBase into the getter twice, rather than passing
> thisValue & slotBase. If you are going to pass two base values in to the
> getter (which, again, I don't think we should - it's unnecessary,
> inefficient, and error-prone), then you should make the If we are going to
> pass the slotBase in to getters, then I think we should make the thisValue
> actually be the thisValue, and not a second copy of slotBase.

Right. So I was planning to pass the same parameters to custom value/accessor getter in the subsequent patch[1].

[1]: https://bugs.webkit.org/show_bug.cgi?id=158014

> 
> I must admit I don't yet understand why this change was necessary as a part
> of a fix to this bug – I'll take a closer look at the test case.

[Yusuke Suzuki]

Reconsider about slotBase.

[Yusuke Suzuki]

(In reply to comment #11)
> Now, I'm attempting to discuss with Saam about performProxyGet on IRC
> (Maybe, now PST is super late, right?). This is only the function that
> requires the both currently.
> And if we can remove this custom accessor usage, we can clean up.

I guess that CustomAccessor is used because throwing an error in getOwnPropertySlot for InternalMethodType::Get is not allowed/considered in the current JSC. slot.getValue() is guarded by exception checks. But getOwnPropertySlot with InternalMethodType::Get is not.
ProxyObject's performProxyGet need to call handlers, thus it can throw.

[Yusuke Suzuki]

(In reply to comment #13)
> (In reply to comment #11)
> > Now, I'm attempting to discuss with Saam about performProxyGet on IRC
> > (Maybe, now PST is super late, right?). This is only the function that
> > requires the both currently.
> > And if we can remove this custom accessor usage, we can clean up.
> 
> I guess that CustomAccessor is used because throwing an error in
> getOwnPropertySlot for InternalMethodType::Get is not allowed/considered in
> the current JSC. slot.getValue() is guarded by exception checks. But
> getOwnPropertySlot with InternalMethodType::Get is not.
> ProxyObject's performProxyGet need to call handlers, thus it can throw.

There is two ways to fix the situation I think.

1. 
Adding a new types, GetCustomValueFunc and GetCustomAccessorFunc.

EncodedJSValue (*GetCustomValueFunc)(ExecState*, EncodedJSValue thisValue, PropertyName, JSObject* slotBase);
EncodedJSValue (*GetCustomAccessorFunc)(ExecState*, EncodedJSValue thisValue, PropertyName);

While GetCustomValueFunc gets slotBase and receiver, GetCustomAccessorFunc does not. And change ProxyObject's custom accessor getter to custom value getter.
Since current JSBoundSlotBaseFunction is not used for custom value getter, this also solves that problem.

2.
Change to allow throwing exceptions from InternalMethodType::Get, and insert exception checks carefully.

[Yusuke Suzuki]

(In reply to comment #14)
> 1. 
> Adding a new types, GetCustomValueFunc and GetCustomAccessorFunc.
> 
> EncodedJSValue (*GetCustomValueFunc)(ExecState*, EncodedJSValue thisValue,
> PropertyName, JSObject* slotBase);
> EncodedJSValue (*GetCustomAccessorFunc)(ExecState*, EncodedJSValue
> thisValue, PropertyName);
> 
> While GetCustomValueFunc gets slotBase and receiver, GetCustomAccessorFunc
> does not. And change ProxyObject's custom accessor getter to custom value
> getter.
> Since current JSBoundSlotBaseFunction is not used for custom value getter,
> this also solves that problem.

Here, I mean that adding these types and replacing GetValueFunc with them. Using the same type for two different functions (custom value func / custom accessor func) may be error prone I think.

> 
> 2.
> Change to allow throwing exceptions from InternalMethodType::Get, and insert
> exception checks carefully.

[Yusuke Suzuki]

*** Bug 157083 has been marked as a duplicate of this bug. ***

[Gavin Barraclough]

> There is two ways to fix the situation I think.
> 
> 1. 
> Adding a new types, GetCustomValueFunc and GetCustomAccessorFunc.
> 
> EncodedJSValue (*GetCustomValueFunc)(ExecState*, EncodedJSValue thisValue,
> PropertyName, JSObject* slotBase);
> EncodedJSValue (*GetCustomAccessorFunc)(ExecState*, EncodedJSValue
> thisValue, PropertyName);
> 
> While GetCustomValueFunc gets slotBase and receiver, GetCustomAccessorFunc
> does not. And change ProxyObject's custom accessor getter to custom value
> getter.
> Since current JSBoundSlotBaseFunction is not used for custom value getter,
> this also solves that problem.

So, Yes, and Yes.

Yes – I think this is just a good idea anyway – had been considering this, and agree that it would make things less confusing.

And Yes – I think this would be an acceptable solution. I am more concerned with the issues around the accessors and this solves that. For custom values reintroducing the slotBase for now is not too horrible.
 
> 2.
> Change to allow throwing exceptions from InternalMethodType::Get, and insert
> exception checks carefully.

I actually think this is probably more the right direction in the long term. Having the extra hop of setting up & calling the custom value getter if likely introduces some avoidable overhead & complexity. But that's a discussion for later - reintroducing a separate slotBase for custom values may well be a pragmatic solution for now.




A bunch more suggestions, for bonus points:


(1) Argument order of your GetCustomValueFunc:

    EncodedJSValue (*GetCustomValueFunc)(ExecState*, EncodedJSValue thisValue, PropertyName, JSObject* slotBase);

I'd personally prefer it if you could change the order to:

    EncodedJSValue (*GetCustomValueFunc)(ExecState*, JSObject* slotBase, PropertyName, EncodedJSValue thisValue);

My reasoning is: for all other existing custom value getters in the code base other than performProxyGet, the this value the getter should be working on is the slotBase. I think the API design here can lead people towards making the right decision. Placing the things they're likely to want as the 1st & 2nd arguments, putting the rarely used things in 3rd & 4th positions seems more logical. Placing the things that most clients want in 1st & 4th positions, with a red herring of a 2nd argument, seems prone to leading to mistakes being made.


(2) Pretty sure custom accessors don't need property names.

If you split the GetValueFunc like this, I think you'll find that no custom accessor uses the PropertyName – we should be able to remove it.

    EncodedJSValue (*GetCustomAccessorFunc)(ExecState*, EncodedJSValue thisValue);


(3) Naming of the arguments.

I think 'thisValue' and 'slotBase' are confusing and unclear names – it would be great to rename them.

- 'thisValue' is the object upon which the access originates. I think both the specification and V8 refer to this as the 'receiver'. I think this is both slightly more descriptive and meaningful than 'thisValue', and matching the spec provides some helpful documentation.
- 'slotBase' is the object that logically holds the property - where the descriptor is read from. I believe V8 uses the term 'holder', and the pec uses 'target'. I like our name 'slotBase' the least. [It says we filled it out into the property slot, it doesn't say why – it can be interpreted in a way that would be describing the receiver, it's just unclear.] 'holder' is not a bad name at all, but I'd be inclined to just match the spec for documentation purposes.

As such, I'd suggest the following function signatures could be ideal:

    EncodedJSValue (*GetCustomValueFunc)(ExecState*, JSObject* target, PropertyName, EncodedJSValue receiver);
    EncodedJSValue (*GetCustomAccessorFunc)(ExecState*, EncodedJSValue receiver);


(4) What about PutValueFunc?

If we're going to make this change to get, ideally we'd symmetrically make it for put – it has the same ambiguity over the this value:

    bool (*PutCustomValueFunc)(ExecState*, JSObject* target, EncodedJSValue value);
    bool (*PutCustomAccessorFunc)(ExecState*, EncodedJSValue receiver, EncodedJSValue value);

[Saam Barati]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Now, I'm attempting to discuss with Saam about performProxyGet on IRC
> > > (Maybe, now PST is super late, right?). This is only the function that
> > > requires the both currently.
> > > And if we can remove this custom accessor usage, we can clean up.
> > 
> > I guess that CustomAccessor is used because throwing an error in
> > getOwnPropertySlot for InternalMethodType::Get is not allowed/considered in
> > the current JSC. slot.getValue() is guarded by exception checks. But
> > getOwnPropertySlot with InternalMethodType::Get is not.
> > ProxyObject's performProxyGet need to call handlers, thus it can throw.
> 
> There is two ways to fix the situation I think.
> 
> 1. 
> Adding a new types, GetCustomValueFunc and GetCustomAccessorFunc.
> 
> EncodedJSValue (*GetCustomValueFunc)(ExecState*, EncodedJSValue thisValue,
> PropertyName, JSObject* slotBase);
> EncodedJSValue (*GetCustomAccessorFunc)(ExecState*, EncodedJSValue
> thisValue, PropertyName);
> 
> While GetCustomValueFunc gets slotBase and receiver, GetCustomAccessorFunc
> does not. And change ProxyObject's custom accessor getter to custom value
> getter.
> Since current JSBoundSlotBaseFunction is not used for custom value getter,
> this also solves that problem.
> 
> 2.
> Change to allow throwing exceptions from InternalMethodType::Get, and insert
> exception checks carefully.

I think (2) would be a very easy change to make. I don't know
of any places that have InternalMethodType::Get that don't immediately
get the value (and if there are, we can probably extend them to work
with a different model, for example, our IC code probably wants caching
information while performing a get()). We can just provide one function that does this get for us and that function can have the exception checks in the correct place. It may be worth doing this for the other InternalMethodType operations as well.

Regarding Gavin's name change proposal, I think that's one of the most important
changes we can make to help this code regain readability and help prevents bugs.
I'm still confused every time I look at this code. I'm confused just reading the comments and I have to remind myself what each name means.
FWIW, my vote is for the 'receiver', 'holder' pair of names. Those names make sense to me and I think it would be hard to confuse them. 

(I'm typing on my phone so I will respond in more detail when I have a keyboard.)

[Yusuke Suzuki]

(In reply to comment #18)
> I think (2) would be a very easy change to make. I don't know
> of any places that have InternalMethodType::Get that don't immediately
> get the value (and if there are, we can probably extend them to work
> with a different model, for example, our IC code probably wants caching
> information while performing a get()). We can just provide one function that
> does this get for us and that function can have the exception checks in the
> correct place. It may be worth doing this for the other InternalMethodType
> operations as well.
> 
> Regarding Gavin's name change proposal, I think that's one of the most
> important
> changes we can make to help this code regain readability and help prevents
> bugs.
> I'm still confused every time I look at this code. I'm confused just reading
> the comments and I have to remind myself what each name means.
> FWIW, my vote is for the 'receiver', 'holder' pair of names. Those names
> make sense to me and I think it would be hard to confuse them. 
> 
> (I'm typing on my phone so I will respond in more detail when I have a
> keyboard.)

Nice! While taking (2), Gavin's proposal is also very good to add it.

(In reply to comment #17)
> (1) Argument order of your GetCustomValueFunc:
> 
>     EncodedJSValue (*GetCustomValueFunc)(ExecState*, EncodedJSValue
> thisValue, PropertyName, JSObject* slotBase);
> 
> I'd personally prefer it if you could change the order to:
> 
>     EncodedJSValue (*GetCustomValueFunc)(ExecState*, JSObject* slotBase,
> PropertyName, EncodedJSValue thisValue);
> 
> My reasoning is: for all other existing custom value getters in the code
> base other than performProxyGet, the this value the getter should be working
> on is the slotBase. I think the API design here can lead people towards
> making the right decision. Placing the things they're likely to want as the
> 1st & 2nd arguments, putting the rarely used things in 3rd & 4th positions
> seems more logical. Placing the things that most clients want in 1st & 4th
> positions, with a red herring of a 2nd argument, seems prone to leading to
> mistakes being made.

Yeah, right. The custom value getter should work with the slot base (holder).
And after taking (2) change, this custom value getter signature should be,

EncodedJSValue (*GetCustomValueFunc)(ExecState*, JSObject* holder, PropertyName);

I think this is reasonable. BTW, it is super slightly efficient in ARM 32bit environment (due to EABI).

> 
> 
> (2) Pretty sure custom accessors don't need property names.
> 
> If you split the GetValueFunc like this, I think you'll find that no custom
> accessor uses the PropertyName – we should be able to remove it.
> 
>     EncodedJSValue (*GetCustomAccessorFunc)(ExecState*, EncodedJSValue
> thisValue);

Yeah, right. Roughly looking into the code base, it seems that there is no custom accessor getter that uses this property name.
Due to the similar reason to the slot base argument, it is better dropping this argument too.
It gives us simplified signature for custom accessor.

EncodedJSValue (*GetCustomAccessorFunc)(ExecState*, EncodedJSValue receiver);


> (3) Naming of the arguments.
> 
> I think 'thisValue' and 'slotBase' are confusing and unclear names – it
> would be great to rename them.

Yup.

> 
> - 'thisValue' is the object upon which the access originates. I think both
> the specification and V8 refer to this as the 'receiver'. I think this is
> both slightly more descriptive and meaningful than 'thisValue', and matching
> the spec provides some helpful documentation.

Strongly agreed. "receiver" seems better.
And by aligning to the spec, we can easily validate our implementation to the spec.
Changing PropertySlot::thisValue to PropertySlot::receiver also gives us better readability.

> - 'slotBase' is the object that logically holds the property - where the
> descriptor is read from. I believe V8 uses the term 'holder', and the pec
> uses 'target'. I like our name 'slotBase' the least. [It says we filled it
> out into the property slot, it doesn't say why – it can be interpreted in a
> way that would be describing the receiver, it's just unclear.] 'holder' is
> not a bad name at all, but I'd be inclined to just match the spec for
> documentation purposes.
> 
> As such, I'd suggest the following function signatures could be ideal:
> 
>     EncodedJSValue (*GetCustomValueFunc)(ExecState*, JSObject* target,
> PropertyName, EncodedJSValue receiver);
>     EncodedJSValue (*GetCustomAccessorFunc)(ExecState*, EncodedJSValue
> receiver);

Personally I like "holder" and "slotBase" both.

> 
> (4) What about PutValueFunc?
> 
> If we're going to make this change to get, ideally we'd symmetrically make
> it for put – it has the same ambiguity over the this value:
> 
>     bool (*PutCustomValueFunc)(ExecState*, JSObject* target, EncodedJSValue
> value);
>     bool (*PutCustomAccessorFunc)(ExecState*, EncodedJSValue receiver,
> EncodedJSValue value);

Yes, I think we should change that.

bool (*PutCustomValueFunc)(ExecState*, JSObject* holder, EncodedJSValue value);
bool (*PutCustomAccessorFunc)(ExecState*, EncodedJSValue receiver, EncodedJSValue value);

[Yusuke Suzuki]

BTW, just out of curiosity, it seems that DOM attributes are represented as custom accessors. Where is this behavior defined?

[Gavin Barraclough]

(In reply to comment #20)
> BTW, just out of curiosity, it seems that DOM attributes are represented as
> custom accessors. Where is this behavior defined?

It certainly is spec defined, but I never remember what is covered in which spec. cdumez would know!

[Yusuke Suzuki]

(In reply to comment #21)
> (In reply to comment #20)
> > BTW, just out of curiosity, it seems that DOM attributes are represented as
> > custom accessors. Where is this behavior defined?
> 
> It certainly is spec defined, but I never remember what is covered in which
> spec. cdumez would know!

Thanks, gotcha.

OK, as a first step, I'll upload the patch that change ProxyObject's custom accessor use and drop the JSObject* slotBase last parameter. It allows Gavin's patch landed.

And after that, I'll replace "thisValue" / "thisObject" etc to "receiver", "receiverObject" etc. And introduce new value / accessor funcs.

[Yusuke Suzuki]

Created attachment 280026 [details]
Patch

Need tests

[Yusuke Suzuki]

Created attachment 280032 [details]
Patch

WIP: dynamic get_from_scope tests are added

[Yusuke Suzuki]

hmmmmmmm, there are so many missing error checks after hasProperty()...
I'll fix this issue in the separate patch...

[Yusuke Suzuki]

Created attachment 280451 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 280548 [details]
Patch

[Yusuke Suzuki]

Performance looks neutral or slightly better. (Due to exec->vm() inlining)

Benchmark report for SunSpider, Octane, Kraken, and AsmBench on hanayamata.

VMs tested:
"baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/slotbase-master/Release/bin/jsc
"patched" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/slotbase3/Release/bin/jsc

Collected 10 samples per benchmark/VM, with 10 VM invocations per benchmark. Emitted a call to gc() between sample
measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to
get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds.

                                                 baseline                  patched                                      
SunSpider:
   3d-cube                                    6.2314+-0.4959     ?      6.2809+-0.5050        ?
   3d-morph                                  25.8342+-0.1299           25.7833+-0.1167        
   3d-raytrace                                7.2051+-0.7738            7.0978+-0.7920          might be 1.0151x faster
   access-binary-trees                        2.3703+-0.1982     ?      2.4647+-0.3670        ? might be 1.0399x slower
   access-fannkuch                            6.3885+-0.2690     ?      6.5939+-0.4993        ? might be 1.0321x slower
   access-nbody                               2.8629+-0.0243     ?      2.8881+-0.0169        ?
   access-nsieve                              3.2344+-0.0862     ?      3.4032+-0.2429        ? might be 1.0522x slower
   bitops-3bit-bits-in-byte                   1.2270+-0.0908            1.2153+-0.0965        
   bitops-bits-in-byte                        2.7954+-0.2056            2.7179+-0.1588          might be 1.0285x faster
   bitops-bitwise-and                         1.9922+-0.0413            1.9755+-0.0188        
   bitops-nsieve-bits                         3.2374+-0.2751     ?      3.3523+-0.3541        ? might be 1.0355x slower
   controlflow-recursive                      2.8321+-0.2692     ?      2.9626+-0.2102        ? might be 1.0461x slower
   crypto-aes                                 5.2610+-0.0625            5.2607+-0.0479        
   crypto-md5                                 2.7231+-0.0911            2.7069+-0.0746        
   crypto-sha1                                2.5471+-0.1121     ?      2.6289+-0.2780        ? might be 1.0321x slower
   date-format-tofte                         10.9906+-0.2385     ?     10.9928+-0.2194        ?
   date-format-xparb                          6.0336+-0.1793            5.8869+-0.0795          might be 1.0249x faster
   math-cordic                                3.0885+-0.0806            3.0579+-0.0693          might be 1.0100x faster
   math-partial-sums                         11.2670+-1.4539           10.5288+-0.0349          might be 1.0701x faster
   math-spectral-norm                         2.1292+-0.0718     ?      2.1982+-0.0945        ? might be 1.0324x slower
   regexp-dna                                 7.3740+-0.0438     ?      7.3743+-0.0397        ?
   string-base64                              3.9361+-0.0184     ?      4.0377+-0.1214        ? might be 1.0258x slower
   string-fasta                               6.4778+-0.4643            6.3651+-0.0904          might be 1.0177x faster
   string-tagcloud                            9.8062+-0.0861     ?     10.0663+-0.4486        ? might be 1.0265x slower
   string-unpack-code                        20.3599+-0.2200     ^     19.4320+-0.1070        ^ definitely 1.0477x faster
   string-validate-input                      4.3019+-0.1048     ?      4.3166+-0.0982        ?

   <arithmetic>                               6.2503+-0.0831            6.2149+-0.0734          might be 1.0057x faster

                                                 baseline                  patched                                      
Octane:
   encrypt                                   0.17801+-0.00143    ?     0.17953+-0.00125       ?
   decrypt                                   3.04231+-0.01640          3.03613+-0.01619       
   deltablue                        x2       0.14989+-0.00561    ?     0.15093+-0.00177       ?
   earley                                    0.33671+-0.00096          0.33611+-0.00151       
   boyer                                     5.31995+-0.01633    ?     5.32392+-0.02081       ?
   navier-stokes                    x2       4.80850+-0.06221          4.77989+-0.00261       
   raytrace                         x2       0.93480+-0.01018    ?     0.93678+-0.00513       ?
   richards                         x2       0.09718+-0.00156    ?     0.09748+-0.00118       ?
   splay                            x2       0.39753+-0.00381          0.39713+-0.00342       
   regexp                           x2      18.71190+-0.12091         18.68516+-0.13110       
   pdfjs                            x2      43.54796+-0.61695    ^    42.18400+-0.34963       ^ definitely 1.0323x faster
   mandreel                         x2      48.97782+-0.49090         48.76037+-0.12389       
   gbemu                            x2      39.68383+-2.84287         39.59681+-2.26499       
   closure                                   0.62641+-0.03081          0.61135+-0.00353         might be 1.0246x faster
   jquery                                    8.06943+-0.14904          8.02381+-0.03563       
   box2d                            x2      14.82936+-0.32708    ^    14.35973+-0.08975       ^ definitely 1.0327x faster
   zlib                             x2     375.53504+-17.26765       366.22104+-7.96651         might be 1.0254x faster
   typescript                       x2     816.42893+-8.88835        810.97114+-7.67738       

   <geometric>                               5.94667+-0.05103          5.90495+-0.03067         might be 1.0071x faster

                                                 baseline                  patched                                      
Kraken:
   ai-astar                                   96.826+-2.078      ?      96.969+-2.035         ?
   audio-beat-detection                       46.986+-2.994      ?      47.065+-2.973         ?
   audio-dft                                 123.185+-0.140      ?     123.213+-0.131         ?
   audio-fft                                  37.285+-0.035             37.282+-0.011         
   audio-oscillator                           53.912+-0.310      ^      53.444+-0.087         ^ definitely 1.0088x faster
   imaging-darkroom                           87.680+-0.277             87.536+-0.115         
   imaging-desaturate                         56.483+-0.362      ?      56.575+-0.646         ?
   imaging-gaussian-blur                      74.618+-4.118      ?      81.110+-4.830         ? might be 1.0870x slower
   json-parse-financial                       46.643+-1.173      ^      43.168+-0.431         ^ definitely 1.0805x faster
   json-stringify-tinderbox                   26.427+-0.259             26.164+-0.056           might be 1.0100x faster
   stanford-crypto-aes                        44.335+-0.476             44.119+-0.609         
   stanford-crypto-ccm                        44.383+-2.910      ?      44.737+-2.325         ?
   stanford-crypto-pbkdf2                    112.726+-3.880      ^     106.533+-1.133         ^ definitely 1.0581x faster
   stanford-crypto-sha256-iterative           38.581+-0.450      ^      37.858+-0.215         ^ definitely 1.0191x faster

   <arithmetic>                               63.576+-0.500             63.269+-0.375           might be 1.0049x faster

                                                 baseline                  patched                                      
AsmBench:
   towers.c                                 276.0853+-0.4833     ?    276.2968+-0.5142        ?
   n-body.c                                 917.2735+-8.8774          913.9123+-10.6103       
   float-mm.c                               713.5380+-5.7114     ?    740.2077+-42.1313       ? might be 1.0374x slower
   container.cpp                           3010.2078+-22.2541        3004.6500+-21.1110       
   quicksort.c                              436.2158+-5.0901     ?    453.1074+-30.1013       ? might be 1.0387x slower
   gcc-loops.cpp                           4142.0506+-26.6415    ?   4169.2655+-30.9709       ?
   bigfib.cpp                               453.3693+-6.3037          451.0540+-4.5639        
   hash-map                                 154.4345+-2.7118     ?    155.4508+-1.9327        ?
   dry.c                                    473.0838+-21.6665    ?    475.0469+-15.7381       ?

   <geometric>                              683.3217+-3.9256     ?    689.2355+-5.7139        ? might be 1.0087x slower

                                                 baseline                  patched                                      
Geomean of preferred means:
   <scaled-result>                           35.6448+-0.1386           35.5661+-0.1647          might be 1.0022x faster

[Yusuke Suzuki]

Created attachment 280550 [details]
Patch

Just a build fix for WebCore

[Yusuke Suzuki]

Created attachment 280553 [details]
Patch

binding test rebaselining

[Darin Adler]

Comment on attachment 280553 [details]
Patch

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

> Source/JavaScriptCore/jsc.cpp:1454
> +    String sourceCode = exec->argument(0).toString(exec)->value(exec);
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());

There’s a faster way to write this:

    auto* sourceCodeObject = exec->argument(0).toStringOrNull(exec);
    if (!sourceCodeObject)
        return JSValue::encode(jsUndefined());
    Strings sourceCode = sourceCodeObject->value(exec);

But if we are not trying to do that faster idiom, then I would have suggested using toWTFString rather than writing out toString(exec)->value(exec), which does the same thing. Maybe someone can make a version of toWTFString that does the faster idiom.

> Source/JavaScriptCore/runtime/JSCJSValue.h:293
> +    template<typename Func>
> +    typename std::result_of<Func(bool, PropertySlot&)>::type getPropertySlot(ExecState*, PropertyName, Func) const;
> +    template<typename Func>
> +    typename std::result_of<Func(bool, PropertySlot&)>::type getPropertySlot(ExecState*, PropertyName, PropertySlot&, Func) const;

Would you be willing to use an entire word, instead of "Func"? I also think these are easier to read on a single line rather than with template above typename.

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:288
> +                return JSValue();

Why do we use jsUndefined() in some cases, and JSValue() in others, for the ignored value when there is an exception in the global state? Is one consistently better?

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:294
> +                JSString* result = ropeBuilder.release();

I’d use auto for this.

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:305
> +        JSString* result = jsNontrivialString(&vm, newString);

Ditto.

[Yusuke Suzuki]

Comment on attachment 280553 [details]
Patch

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

>> Source/JavaScriptCore/jsc.cpp:1454
>> +        return JSValue::encode(jsUndefined());
> 
> There’s a faster way to write this:
> 
>     auto* sourceCodeObject = exec->argument(0).toStringOrNull(exec);
>     if (!sourceCodeObject)
>         return JSValue::encode(jsUndefined());
>     Strings sourceCode = sourceCodeObject->value(exec);
> 
> But if we are not trying to do that faster idiom, then I would have suggested using toWTFString rather than writing out toString(exec)->value(exec), which does the same thing. Maybe someone can make a version of toWTFString that does the faster idiom.

Interesting. I'll change this to toWTFString.

>> Source/JavaScriptCore/runtime/JSCJSValue.h:293
>> +    typename std::result_of<Func(bool, PropertySlot&)>::type getPropertySlot(ExecState*, PropertyName, PropertySlot&, Func) const;
> 
> Would you be willing to use an entire word, instead of "Func"? I also think these are easier to read on a single line rather than with template above typename.

Thanks. I renamed it to CallbackWhenNoException. And make this a single line.

>> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:288
>> +                return JSValue();
> 
> Why do we use jsUndefined() in some cases, and JSValue() in others, for the ignored value when there is an exception in the global state? Is one consistently better?

This code itself is derived from the original code's `JSValue()`.
Since this value is ignored when exception occurs, any value is OK.
In the meantime, I'll use jsUndefined for that.

>> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:294
>> +                JSString* result = ropeBuilder.release();
> 
> I’d use auto for this.

OK, fixed.

>> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:305
>> +        JSString* result = jsNontrivialString(&vm, newString);
> 
> Ditto.

Fixed.

[Yusuke Suzuki]

Committed r201703: <http://trac.webkit.org/changeset/201703>