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 ---
Created attachment 278946 [details] Patch
Created attachment 278948 [details] Patch
Created attachment 278951 [details] Patch
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.
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.
Committed r201322: <http://trac.webkit.org/changeset/201322>
*** Bug 154320 has been marked as a duplicate of this bug. ***
Committed r201331: <http://trac.webkit.org/changeset/201331>
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.
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.
(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.
Reconsider about slotBase.
(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.
(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.
(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.
*** Bug 157083 has been marked as a duplicate of this bug. ***
> 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);
(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.)
(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);
BTW, just out of curiosity, it seems that DOM attributes are represented as custom accessors. Where is this behavior defined?
(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!
(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.
Created attachment 280026 [details] Patch Need tests
Created attachment 280032 [details] Patch WIP: dynamic get_from_scope tests are added
hmmmmmmm, there are so many missing error checks after hasProperty()... I'll fix this issue in the separate patch...
Created attachment 280451 [details] Patch WIP
Created attachment 280548 [details] Patch
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
Created attachment 280550 [details] Patch Just a build fix for WebCore
Created attachment 280553 [details] Patch binding test rebaselining
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.
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.
Committed r201703: <http://trac.webkit.org/changeset/201703>