[Webkit-unassigned] [Bug 162124] op_get_by_id_with_this should use inline caching

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=162124

--- Comment #74 from Caio Lima <ticaiolima at gmail.com> ---
Comment on attachment 300268
  --> https://bugs.webkit.org/attachment.cgi?id=300268
Patch Updated

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

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1153
>> +            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;
> 
> I think the above comment makes this code wrong. We should have tests for this. Do you already have tests?
> According to the above comment:
> - CustomValue should be passed the property holder, which might not necessarily be |this|, however, your code says that it is.
> - Custom accessors are indeed passed |this|.
> 
> Please add tests for both of these.

I'm confused now. When we have a customAccessor, thisValue is the receiver, in that case it shouldn't be the "thisGPR"? I created a test to cover the customAccessorGetter:

```
// CustomGetter case

 let customGetter = createCustomGetterObject();
 Object.setPrototypeOf(customGetter, Object.prototype);

 let subObj = {
     __proto__: customGetter,
     get value () {
         return super.customGetterAccessor;
     }
 }

 for (let i = 0; i < 1000000; i++) {
     let value = getterValue(subObj);
     assert(value == 100);
 }

 subObj.shouldThrow = true;
 for (let i = 0; i < 1000000; i++) {
     try {
         getterValue(subObj);
         assert(false);
     } catch(e) {
         assert(e instanceof TypeError);
     };
 }
```

If I don't set baseForCustomValue with thisGPR, this test case will fail. Is that correct behavior?

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2869
>> +        } else if (m_node->child1().useKind() == CellUse || m_node->child2().useKind() == CellUse) {
> 
> This case should now be impossible, please remove.

Nice catch. My bad.

>> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:676
>> +    emitJumpSlowCaseIfNotJSCell(thisVReg, regT4);
> 
> This still has the problem I was talking about in my review of an earlier patch. The slow case does not handle regT3/regT4 not being populated.

My bad =(. Just forgot do modify it before.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170205/b9f48319/attachment.html>