ChakraCore
ChakraCore copied to clipboard
Invalidate `IsInstInlineCache` in `Object.setPrototypeOf`
Using Object.setPrototypeOf may result in instanceof returning the wrong result, as the IsInstInlineCache is not invalidated (See #5915).
Steps:
- Iterate over the existing
function -> IsInstInlineCache[]dictionary using the newThreadContext.MapIsInstInlineCachesfunction - Walk the prototype chain to check for the cached
type - Ignore if
typeisnullhttps://github.com/chakra-core/ChakraCore/blob/0cfe82d202c0298f4bbde06070f8dbf9099946c8/lib/Runtime/Library/JavascriptFunction.cpp#L3365-L3367 - If found, remove from list using
ThreadContext.UnregisterIsInstInlineCache(correctsnext) and ... - Invalidate using
memset
I couldn't come up with a scenario where the function could be affected (x instance of function).
Therefore, the following code should not be necessary (not included in latest commit):
https://github.com/chakra-core/ChakraCore/blob/2d7e459c40cbf12efff28e00811dc768b7d52f65/lib/Runtime/Library/JavascriptObject.cpp#L255-L272
@rhuanjl
Fixes #5915
Thank you for the submission, I'm going to have think a bit more about this (probably on monday)
This certainly fixes the specific case, but I'd like to throw some more complex cases at it AND also see if we can test whether it's removing caches it shouldn't (which will be slightly harder to test).
Yes, that's a good idea!
I already tried to figure that out, but I couldn't really figure out, what class is behind a Type in CC (e.g. reading constructor name).
The logic defenently doesn't remove everything all the time.
Sorry that I'm not done with this yet, I've been struggling to think of how to do the additional tests I'd like to do, there is no obvious good way to verify cache hits, so, awkwardly we may need to add one (for debug builds only) in order to test this sufficiently.
No problem, I’m struggling with that as well. What exactly do you mean by “verify cache hits”? How could we do that?
No problem, I’m struggling with that as well. What exactly do you mean by “verify cache hits”? How could we do that?
If you want to try and do it:
- Add a phase for instanceofCache in here: https://github.com/chakra-core/ChakraCore/blob/master/lib/Common/ConfigFlagsList.h
- In
JavascriptFunction::HasInstance(in JavascriptFunction.cpp) add some code inside a#ifdef ENABLE_TEST_HOOKSthat contains anif(PHASE_TRACE1(phase_you_created))containing code to print/log the result of the cache check for instanceof. - With a debug or test build of ch the command line flag
-trace:phase_you_createdwill then trigger that logging code - We could then create a test case using a baseline file to enable us to check that the right things are logged, specifically we'd want cases where some cache should be cleared but some should not be and check it works.
Sorry, that it took me so long, but I still had some work to do. I’ve now replaced the test with a baseline test using the trace output from the previous commit. It would be great if you could check if I missed an interesting case.
The Problem is, that jit abstracts the calls to the test functions away. Therefore, the baseline is "incorrect" in those tests.
EDIT: I'm now preventing the fast path in jit, kind of like it's done in some other scenarios for testing.
I'm not ignoring this - thank you for your efforts, I've still got some more cases I'd like to check and out of time for today will look a bit further when I can.
Apologies for the massive delay - I've paused working on other code changes until I've sorted apple silicon support which is taking me longer than I hoped...
@rhuanjl No problem; just take your time!