Fix Issue#3064/#3423: Cache conflicts in super property access #3336

suwc · 2017-07-13T00:39:39Z

Accesses to super properties are cached on 'this' object (vs. the
"super" object), causing conflict between e.g. super.x and this.x.
Similar conflicts cause Issue#3423 for GetProperty cases.
Fix by adding 'isSuper' flag to use appropriate object for caching.

Fixes #3064, Fixes #3423

suwc · 2017-07-13T00:40:32Z

Addresses #3064

suwc · 2017-07-13T00:41:45Z

@Microsoft/chakra-developers @Microsoft/chakra-es please review. Thanks!

curtisman · 2017-07-17T13:58:37Z

lib/Runtime/Types/TypePropertyCache.cpp

- &prototypeObjectWithProperty))
+ &prototypeObjectWithProperty)
+ || (prototypeObjectWithProperty != nullptr
+ && prototypeObjectWithProperty != propertyObject))


Huh? prototypeObjectWithProperty should never be the propertyObject itself. Otherwise, we will not hit the "proto" inline cache and have prototypeObjectWithProperty not null. There is probably something else wrong.

@suwc , as we discussed, I think we're probably abusing the cache on a byte code level. I'll make some time to investigate today. If that's the case, then we want to fix the code gen rather than working around it in the cache logic.

The TypePropertyCache can be populated in multiple occasions. In this repro case, calls to “super.x” go through these lines in GetProperty_Internal():

PropertyValueInfo::Set(info, requestContext->GetLibrary()->GetMissingPropertyHolder(), 0); CacheOperators::CachePropertyRead(instance, requestContext->GetLibrary()->GetMissingPropertyHolder(), isRoot, propertyId, true, info, requestContext);

Now that the cache is populated with requestContext->GetLibrary()->GetMissingPropertyHolder() as “prototypeObjectWithProperty”, next call to “this.x” (having same “instance”) will be a cache hit (with prototypeObjectWithProperty != nullptr), whereas it should be a cache miss + update.

In reply to: 127714797 [](ancestors = 127714797)

Yes, as I thought, the byte code gen for super.x is misusing the inline caches. So let's apply a fix there rather than in the cache logic. We can discuss offline.

suwc · 2017-07-25T16:46:03Z

@dotnet-bot test Ubuntu _no_jit_shared_ubuntu_linux_test

curtisman · 2017-07-27T18:39:41Z

lib/Runtime/Language/JavascriptOperators.cpp

 if (value && !requestContext->IsUndeclBlockVar(*value) && !WithScopeObject::Is(object))
 {
- CacheOperators::CachePropertyRead(instance, object, isRoot, propertyId, false, info, requestContext);
+ CacheOperators::CachePropertyRead(isSuper ? propertyObject : instance, object, isRoot, propertyId, false, info, requestContext);


I think this should always be propertyObject......

@curtisman I don't have an example where caching on instance is more appropriate when propertyObject != instance. When a super property ("super.x") exists in super object's prototype (vs. the super object itself), it is distinguished by object != prototypeObject. It would be simpler if we can go without 'isSuper'.

@pleath am I missing something?

You should be able to find all the cases that caching on the instance is wrong and create test cases for them

If we can prove that there's no case where propertyObject != instance, and where we want to cache based on instance, or where the fact that propertyObject !=instance is our indication that we have a prototype access, then going without isSuper seems fine. The only case I can think of is where instance is a tagged number, and in that case it seems fine to cache as a local access to Number.prototype (which will be propertyObject, right?).

(All of which means that the logic we've had since IE9 is subtly wrong, but in a way that never mattered until super was implemented. That may be true, but it would be nice to prove it.)

Besides super property access, other cases where propertyObject != instance are following:

instance is proxy, propertyObject is target object

instance is number, propertyObject is Number.prototype

instance is boolean, propertyObject is Boolean.prototype

By design, 1) does not cache property access (confirmed) because underlying target object can mutate without proxy knowing it.

and 3) are justified to cache on their prototype (propertyObject).

In reply to: 130143456 [](ancestors = 130143456)

akroshg · 2017-07-29T00:00:01Z

so what is a state on this PR?

pleath · 2017-07-30T15:47:54Z

A simpler fix along the lines of what Curtis suggests seems possible, but we want to do some work to confirm that.

suwc · 2017-08-08T16:39:08Z

Besides super property access, other cases where propertyObject != instance are following:

instance is proxy, propertyObject is target object
instance is number, propertyObject is Number.prototype
instance is boolean, propertyObject is Boolean.prototype

By design, 1) does not cache property access (confirmed) because underlying target object can mutate without proxy knowing it.

and 3) are justified to cache on their prototype (propertyObject).

pleath · 2017-08-09T00:36:56Z

lib/Backend/GlobOpt.cpp

 if (opnd->IsSymOpnd() && opnd->AsSymOpnd()->IsPropertySymOpnd())
 {
- TryOptimizeInstrWithFixedDataProperty(&instr);
+ if (instr->m_opcode != Js::OpCode::LdSuperFld) // TODO: should profile LdSuperFld?


Yes, I would prefer to profile LdSuperFld. It seems best to let it participate in this optimization. If there's a reason we can't do that, I'd prefer to use some kind of general check on the opcode attributes rather than special-casing for this opcode. #Pending

pleath · 2017-08-09T00:37:52Z

lib/Backend/JnHelperMethodList.h


 HELPERCALL(Op_PatchGetValue, ((Js::Var (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId))Js::JavascriptOperators::PatchGetValue<true, Js::InlineCache>), AttrCanThrow)
-HELPERCALL(Op_PatchGetValueWithThisPtr, ((Js::Var(*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var))Js::JavascriptOperators::PatchGetValueWithThisPtr<true, Js::InlineCache>), AttrCanThrow)
+HELPERCALL(Op_PatchGetSuperValue, ((Js::Var(*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, const bool))Js::JavascriptOperators::PatchGetSuperValue<true, Js::InlineCache>), AttrCanThrow)


Curious...is there a reason for the name change? I kind of like the generic name better than one that suggests this is only for super. But I could be convinced. #Resolved

This is just a wrapper. I renamed the generic version to "PatchGetValueHelper" and hid it from JIT, in order to have (current and future) flexibility of adding, changing, or removing a parameter without JIT impact. Same goes for other helper functions. JIT doesn't need to deal with these.

In reply to: 132067176 [](ancestors = 132067176)

Well, the JIT isn't really a factor. I'm just wondering why we wouldn't go with a generic name -- or, specifically, why we would change from using a generic name to using a less portable one. Can we revert this?

pleath · 2017-08-09T00:40:49Z

lib/Runtime/ByteCode/ByteCodeWriter.cpp

 }

- void ByteCodeWriter::PatchablePropertyWithThisPtr(OpCode op, RegSlot value, RegSlot instance, RegSlot thisInstance, uint cacheId, bool isCtor, bool registerCacheIdForCall)
+ void ByteCodeWriter::PatchableSuperProperty(OpCode op, RegSlot value, RegSlot instance, RegSlot thisInstance, uint cacheId, bool isCtor, bool registerCacheIdForCall)


Again, I generally prefer a generic name. #Pending

Although this so far is used only for super, I can see a point for leaving it in a generic name here. I will revert it to the old name.

In reply to: 132067519 [](ancestors = 132067519)

suwc · 2017-08-09T18:54:41Z

@pleath, latest iteration made LdSuperFld & StSuperFld ProfiledInstr. Also reverted name changes in bytecode emitter to remain generic.

pleath · 2017-08-11T01:09:05Z

lib/Backend/JnHelperMethodList.h


 HELPERCALL(Op_PatchPutValue, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutValue<true, Js::InlineCache>), AttrCanThrow)
-HELPERCALL(Op_PatchPutValueWithThisPtr, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutValueWithThisPtr<true, Js::InlineCache>), AttrCanThrow)
+HELPERCALL(Op_PatchPutSuperValue, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutSuperValue<true, Js::InlineCache>), AttrCanThrow)


Ditto for these name changes. The existing names are preferable, IMO, because they describe what the method does and don't create the impression that the methods are necessarily limited to one opcode.

…cess Accesses to super properties are cached on 'this' object (vs. the "super" object), causing conflict between e.g. super.x and this.x. Similar conflicts cause Issue#3423 for GetProperty cases. Fix by changing to object/propertyObject for caching super property accesses.

suwc · 2017-08-11T17:16:22Z

@pleath, I reverted the name change. Thanks!

pleath · 2017-08-11T17:43:39Z

Awesome. Thanks. That leaves us with a nice, simple-looking change.

…operty access Merge pull request #3336 from suwc:build/suwc/Issue3064 Accesses to super properties are cached on 'this' object (vs. the "super" object), causing conflict between e.g. super.x and this.x. Similar conflicts cause Issue#3423 for GetProperty cases. Fix by adding 'isSuper' flag to use appropriate object for caching. Fixes #3064, Fixes #3423

…n super property access Merge pull request #3336 from suwc:build/suwc/Issue3064 Accesses to super properties are cached on 'this' object (vs. the "super" object), causing conflict between e.g. super.x and this.x. Similar conflicts cause Issue#3423 for GetProperty cases. Fix by adding 'isSuper' flag to use appropriate object for caching. Fixes #3064, Fixes #3423

…e conflicts in super property access Merge pull request #3336 from suwc:build/suwc/Issue3064 Accesses to super properties are cached on 'this' object (vs. the "super" object), causing conflict between e.g. super.x and this.x. Similar conflicts cause Issue#3423 for GetProperty cases. Fix by adding 'isSuper' flag to use appropriate object for caching. Fixes #3064, Fixes #3423

msftclas added the cla-not-required label Jul 13, 2017

suwc requested review from boingoing and pleath July 13, 2017 00:40

curtisman reviewed Jul 17, 2017

View reviewed changes

suwc force-pushed the build/suwc/Issue3064 branch from 990d7ec to f2d0e2e Compare July 25, 2017 07:27

suwc changed the title ~~Fix Issue#3064: TypePropertyCache Miss with propertyObject~~ Fix Issue#3064/#3423: Cache conflicts in super property access Jul 25, 2017

curtisman reviewed Jul 27, 2017

View reviewed changes

suwc force-pushed the build/suwc/Issue3064 branch from f2d0e2e to 867e4be Compare August 8, 2017 06:58

pleath reviewed Aug 9, 2017

View reviewed changes

suwc force-pushed the build/suwc/Issue3064 branch from 867e4be to abba9de Compare August 9, 2017 18:50

pleath reviewed Aug 11, 2017

View reviewed changes

suwc force-pushed the build/suwc/Issue3064 branch from abba9de to d2a1cae Compare August 11, 2017 17:15

pleath approved these changes Aug 11, 2017

View reviewed changes

chakrabot merged commit d2a1cae into chakra-core:release/1.6 Aug 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix Issue#3064/#3423: Cache conflicts in super property access #3336

Fix Issue#3064/#3423: Cache conflicts in super property access #3336

Uh oh!

suwc commented Jul 13, 2017 •

edited by dilijev

Loading

suwc commented Jul 13, 2017

suwc commented Jul 13, 2017

curtisman Jul 17, 2017 •

edited

Loading

pleath Jul 17, 2017

suwc Jul 17, 2017

pleath Jul 19, 2017

suwc commented Jul 25, 2017

curtisman Jul 27, 2017

suwc Jul 27, 2017

curtisman Jul 28, 2017

pleath Jul 28, 2017

pleath Jul 28, 2017

suwc Aug 8, 2017

akroshg commented Jul 29, 2017

pleath commented Jul 30, 2017

suwc commented Aug 8, 2017

pleath Aug 9, 2017 •

edited by suwc

Loading

pleath Aug 9, 2017 •

edited by suwc

Loading

suwc Aug 9, 2017

pleath Aug 11, 2017

pleath Aug 9, 2017 •

edited by suwc

Loading

suwc Aug 9, 2017

suwc commented Aug 9, 2017

pleath Aug 11, 2017

suwc commented Aug 11, 2017

pleath commented Aug 11, 2017

Labels

6 participants

Fix Issue#3064/#3423: Cache conflicts in super property access #3336

Fix Issue#3064/#3423: Cache conflicts in super property access #3336

Uh oh!

Conversation

suwc commented Jul 13, 2017 • edited by dilijev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

suwc commented Jul 13, 2017

suwc commented Jul 13, 2017

curtisman Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suwc commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akroshg commented Jul 29, 2017

pleath commented Jul 30, 2017

suwc commented Aug 8, 2017

pleath Aug 9, 2017 • edited by suwc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

pleath Aug 9, 2017 • edited by suwc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleath Aug 9, 2017 • edited by suwc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suwc commented Aug 9, 2017

Choose a reason for hiding this comment

suwc commented Aug 11, 2017

pleath commented Aug 11, 2017

Labels

6 participants

suwc commented Jul 13, 2017 •

edited by dilijev

Loading

curtisman Jul 17, 2017 •

edited

Loading

pleath Aug 9, 2017 •

edited by suwc

Loading

pleath Aug 9, 2017 •

edited by suwc

Loading

pleath Aug 9, 2017 •

edited by suwc

Loading