[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