[webkit-reviews] review granted: [Bug 178067] [JSC] __proto__ getter should be fast : [Attachment 323308] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 16 11:33:32 PDT 2017


Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 178067: [JSC] __proto__ getter should be fast
https://bugs.webkit.org/show_bug.cgi?id=178067

Attachment 323308: Patch

https://bugs.webkit.org/attachment.cgi?id=323308&action=review




--- Comment #13 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 323308
  --> https://bugs.webkit.org/attachment.cgi?id=323308
Patch

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

r=me with some nits and a comment regarding IntrinsicGetters

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2706
> +	       bool cannotFold = false;

style nit: I think these variables are easier to read if they say what they
"can" do vs "cannot" do. So, I think it'd be easier if we called this "canFold"
and flipped all the assignments to it.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2726
> +	       if (prototype && !cannotFold) {

so, for example, I think it's easier to read this as
prototype && canFold
instead of
prototype && !cannotFold

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3060
> +	   variant.structureSet().forEach([&] (Structure* structure) {

is this set guaranteed to be non-empty?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10874
> +	   callOperation(operationGetPrototypeOf, resultRegs,
JSValueRegs::payloadOnly(valueGPR));

Is it worth having a version of the function where the operand is already a
JSObject*?

> Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:141
> +	   jit.loadValue(MacroAssembler::Address(baseGPR,
sizeof(EncodedJSValue) * structure()->polyProtoOffset() +
JSObject::offsetOfInlineStorage()), valueRegs);
> +	   state.succeed();
> +	   return;

I bet this is dead code since Repatch.cpp doesn't emit intrinsic getters with
poly proto. The reason for this is that I didn't make it work with the typed
array code. So if you want this to work with poly proto, I think you need to
change things in Repatch.cpp to get it to emit these. You may want to disable
it for typed arrays ATM. Or have canEmitIntrinsicGetter know if it's a poly
proto access or not. The reason this isn't trivial to do is take this example:
```
function foo(o) {
    return o.__proto__;
}
let o = somethingPolyProto;
assert(o.__proto__ === Object.prototype);
assert(foo(o) === Object.prototype);
o.__proto__ = null;
assert(foo(o) === undefined)
```

I think your code would break on this case, since you're not verifying the
prototype chain. So, I think it's worth skipping poly proto for now.


More information about the webkit-reviews mailing list