Fix bug in CheckIfTypeIsEquivalent helper #1368

kunalspathak · 2016-08-02T01:34:27Z

CheckIfTypeIsEquivalent helper is called from JITted code to perform equivalent type check.
It validates if the object's type is present in one of the 8 cached equivalent
types and if yes, updates the guard value and return back to JITted code.
If not, it takes slow path and verifies the prototype of object to that of cached
type. The cached type was always fetched from 0th index of array of cached equivalent types.
If this was nullptr, we were returning back to JITted code saying the type of object is not
equivalent to any cached types and then JITted code would bailout.
Some of the type caches can get collected in PreSweepCallback if types are not marked,
hence the assumption to fetch cached type from 0th index is not valid because it might have
got collected while other cached types in the array are still around. Hence we should
fetch a non null equivalent type from cached array of types and then do subsequent slow
path checks.

This change reduces the bailouts of kind BailOutFailedEquivalentTypeCheck significantly.

Perf: ~~In progress~~ Shows 2.5% win in acmeair. Others are flat.
Test : Unit test pass, internal tools in progress.

kunalspathak · 2016-08-02T01:35:08Z

FYI - @LouisLaf , @pleath , @rajatd

satheeshravi · 2016-08-02T01:43:36Z

    if (cache->IsLoadedFromProto() && type->GetPrototype() != refType->GetPrototype())

@pleath, Why aren't we checking all non-null cached type in the slow path? Is it better off to bailout ?

Refers to: lib/Runtime/Language/JavascriptOperators.cpp:8247 in 7ce8997. [](commit_id = 7ce8997, deletion_comment = False)

pleath · 2016-08-02T02:25:42Z

IIRC the type in slot 0 is the one that's checked by the online jitted code. If that's true it should always hold the last type we saw, and we should never allow it to become null. Is that not the case?

-- Paul

From: Satheesh Kumar Ravindranathmailto:[email protected]
Sent: ‎8/‎1/‎2016 6:43 PM
To: Microsoft/ChakraCoremailto:[email protected]
Cc: Paul Leathersmailto:[email protected]; Mentionmailto:[email protected]
Subject: Re: [Microsoft/ChakraCore] Fix bug in CheckIfTypeIsEquivalent helper (#1368)

if (cache->IsLoadedFromProto() && type->GetPrototype() != refType->GetPrototype())

@pleathhttps://github.com/pleath, Why aren't we checking all non-null cached type in the slow path? Is it better off to bailout ?

Refers to: lib/Runtime/Language/JavascriptOperators.cpp:8247 in 7ce89977ce8997. <commit_id%20=%207ce8997c00764694c085e9dfefdbbedcdc894f18,%20deletion_comment%20=%20False>

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/1368#issuecomment-236767820, or mute the threadhttps://github.com/notifications/unsubscribe-auth/APF8RO9TEWo-Bs83wS68N_3-qFMbqR8Mks5qbqDVgaJpZM4JaKJF.

kunalspathak · 2016-08-02T16:49:48Z

    if (cache->IsLoadedFromProto() && type->GetPrototype() != refType->GetPrototype())

AFAIK, cached types would have same prototype/typeId (and thats why there are equivalent types). I can add an assert to ensure this is the case.

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

Refers to: lib/Runtime/Language/JavascriptOperators.cpp:8247 in 7ce8997. [](commit_id = 7ce8997, deletion_comment = False)

rajatd · 2016-08-02T19:05:16Z

I think we should look into why the caches are being zeroed.

@kunalspathak , you mentioned the caches can get zeroed if the types weren't marked - that's weird because we pin all the types in an equivalent set at work item creation time. Right now the equivalent type cache can hold a maximum of 8 types, but we have been talking about making it growable. It would be nice to not have to loop through the cache for the first non-null entry.

@pleath - while you are correct that the jitted code checks against the first type in the equivalent set, but if I read the code correctly, it doesn't index into the equivalent type cache or set to get the first type. It loads it from the guard, and we do record the last seen type on the guard (if it was, in fact, equivalent to the other types).

kunalspathak · 2016-08-02T19:28:07Z

@pleath , you are right, the guard value in online JITted code is same as type in slot[0]. So ideally equivTypes[0] == guard->GetValue(). However if you see below, in slow path, we will overwrite the guard value with newer type. At the same time, we knock off one of the cached type (for eg. at index 5) and replace it with current type. When this happens, our assumption that guard value is always from slot 0 is invalidated. This might answer Rajat's question on why we collect the old type that was knocked off from cached type and now no longer exist as a guard value.
Next time we come in CheckIfTypeIsEquivalent, the guard value is the one that was present at index 5, however we hard code and check the type at slot 0 which might have got collected now. Hence we should get the non-null type from cached types.

But after re-reading the code and I think instead of looping to fetch the refType, I could simply use the guard value as refType to check for prototype/typeid.

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

agarwal-sandeep · 2016-08-05T00:17:30Z

        return false;

Now if we reach here that means we don't have any type in equivTypes. Better to put an assert that all types from 1 to < EQUIVALENT_TYPE_CACHE_SIZE are nullptr. Will help in catching issue where compaction failed. #Resolved

Refers to: lib/Runtime/Language/JavascriptOperators.cpp:8236 in d633d84. [](commit_id = d633d84, deletion_comment = False)

agarwal-sandeep · 2016-08-05T00:22:24Z

lib/Runtime/Language/JavascriptOperators.cpp

            {
-                cache->nextEvictionVictim = 0;
+                Output::Print(_u("EquivObjTypeSpec: Saving type in unused slot of equiv types cache. \n"));
+                Output::Flush();
            }


Better to use just one line - OUTPUT_TRACE(Js::EquivObjTypeSpecPhase, _u("EquivObjTypeSpec: Saving type in unused slot of equiv types cache. \n")); #WontFix

Yeah, i will just follow the consistency here and keep it PHASE_TRACE1

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

kunalspathak · 2016-08-06T00:12:25Z

        return false;

done.

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

Refers to: lib/Runtime/Language/JavascriptOperators.cpp:8236 in d633d84. [](commit_id = d633d84, deletion_comment = False)

pleath · 2016-08-08T20:10:22Z

lib/Runtime/Base/FunctionBody.cpp

+        {
+            // just mark this as actual invalidated since there are no types
+            // present
+            guard->Invalidate();


Correct me if this isn't true, but can InvalidateWhileSweeping() not be called when the current guard type is dead but other types in the equivalent cache are live? If that happens, do we really want to invalidate the guard?

It was added more from perf perspective in CheckIfTypeIsEquivalent where first thing that is done is

if (guard->GetValue() == 0) { return false; }

So if the guard was invalidated while sweeping and none of equiv types are alive, we can return right away from CheckIfTypeIsEquivalent. Without this, we would go further and fetch equivTypes[0] and that would be nullptr and that is when we would have returned.

What I'm asking is, what prevents us from invalidating the guard and leaving it null while other equivalent types in the cache are alive?

@kunalspathak pointed out the place where we avoid invalidating if there's still a live type.

pleath · 2016-08-09T19:13:09Z

lib/Runtime/Base/FunctionBody.h

+#if DBG
+            wasReincarnated = true;
+#endif
+            this->value = 2;


Can you replace "2" with a defined constant?

pleath · 2016-08-09T20:32:55Z

kunalspathak · 2016-08-09T22:53:08Z

@dotnet-bot test OSX osx_osx_debug_static please

kunalspathak · 2016-08-09T23:08:13Z

@dotnet-bot test Windows x86_release please

kunalspathak · 2016-08-10T22:40:35Z

@pleath , please review fix for cross context

kunalspathak · 2016-08-10T22:43:47Z

@dotnet-bot test Windows x64_release please

pleath · 2016-08-10T22:45:36Z

👍

kunalspathak · 2016-08-10T23:03:47Z

@dotnet-bot test OSX osx_osx_debug_static please

`CheckIfTypeIsEquivalent` helper is called from JITted code to perform equivalent type check. It validates if the object's type is present in one of the 8 cached equivalent types and if yes, updates the guard value and return back to JITted code. If not, it takes slow path and verifies the prototype of object to that of cached type. However if guard is not valid, we would just bailout `BailOutFailedEquivalentTypeCheck`. Today in `PreSweepCallback` we would invalidate the guard if the underline type is not marked. However we could have invalid guard value but if there are alive types present in equivTypes[], we could use them to check if `type` in question matches any of them and if yes, update the guard value, so next time JIT code will be able to take advantage of it next time. On instrumenting code, noticed that there are high frequency that this happens (at least in acmeair) where we have valid equivTypes[], but we don'take take its advantage just because guard value is invalid. This gives approx. 2.5 % win in acmeair. Other benchmarks are flat. Also, I noticed that the cache eviction policy was kicking out valid caches despite of having empty slots in types[] where new type could have been stored. Modified the eviction logic to take advantage of empty slots before going to existing eviction logic. I have also added logic to compact the equivTypes by moving non-null types towards front of array. Test: Internal tools pass Perf: Standard benchmarks are almost flat with minor improvement in Octane, Kraken. Acme-air shows 2.5% win.

Merge pull request #1368 from kunalspathak:typecheck `CheckIfTypeIsEquivalent` helper is called from JITted code to perform equivalent type check. It validates if the object's type is present in one of the 8 cached equivalent types and if yes, updates the guard value and return back to JITted code. If not, it takes slow path and verifies the prototype of object to that of cached type. However if guard is not valid, we would just bailout `BailOutFailedEquivalentTypeCheck`. Today in `PreSweepCallback` we would invalidate the guard if the underline type is not marked. However we could have invalid guard value but if there are alive types present in equivTypes[], we could use them to check if `type` in question matches any of them and if yes, update the guard value, so next time JIT code will be able to take advantage of it next time. On instrumenting code, noticed that there are high frequency that this happens (at least in acmeair) where we have valid equivTypes[], but we don'take take its advantage just because guard value is invalid. This gives approx. 2.5 % win in acmeair. Other benchmarks are flat. Also, I noticed that the cache eviction policy was kicking out valid caches despite of having empty slots in types[] where new type could have been stored. Modified the eviction logic to take advantage of empty slots before going to existing eviction logic. I have also added logic to compact the equivTypes by moving non-null types towards front of array. Test: Internal tools pass Perf: Standard benchmarks are almost flat with minor improvement in Octane, Kraken. Acme-air shows 2.5% win.

msftclas added the cla-not-required label Aug 2, 2016

agarwal-sandeep reviewed Aug 5, 2016
View reviewed changes

kunalspathak force-pushed the typecheck branch from 47cd35d to cd1e92d Compare August 8, 2016 17:15

pleath reviewed Aug 8, 2016
View reviewed changes

kunalspathak force-pushed the typecheck branch from cd1e92d to 9cf0261 Compare August 9, 2016 16:43

pleath reviewed Aug 9, 2016
View reviewed changes

kunalspathak force-pushed the typecheck branch from f16b28c to 796e2f8 Compare August 9, 2016 23:01

kunalspathak force-pushed the typecheck branch from c8ba729 to 475df2c Compare August 11, 2016 15:59

chakrabot merged commit 475df2c into chakra-core:master Aug 11, 2016

Fix bug in CheckIfTypeIsEquivalent helper #1368

Fix bug in CheckIfTypeIsEquivalent helper #1368

Uh oh!

Conversation

kunalspathak commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kunalspathak commented Aug 2, 2016

Uh oh!

satheeshravi commented Aug 2, 2016

Uh oh!

pleath commented Aug 2, 2016

Uh oh!

kunalspathak commented Aug 2, 2016

Uh oh!

rajatd commented Aug 2, 2016

Uh oh!

kunalspathak commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agarwal-sandeep commented Aug 5, 2016 • edited by kunalspathak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agarwal-sandeep Aug 5, 2016 • edited by kunalspathak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunalspathak Aug 6, 2016

Choose a reason for hiding this comment

Uh oh!

kunalspathak commented Aug 6, 2016

Uh oh!

pleath Aug 8, 2016

Choose a reason for hiding this comment

Uh oh!

kunalspathak Aug 8, 2016

Choose a reason for hiding this comment

Uh oh!

pleath Aug 8, 2016

Choose a reason for hiding this comment

Uh oh!

pleath Aug 9, 2016

Choose a reason for hiding this comment

Uh oh!

pleath Aug 9, 2016

Choose a reason for hiding this comment

Uh oh!

pleath commented Aug 9, 2016

Uh oh!

kunalspathak commented Aug 9, 2016

Uh oh!

kunalspathak commented Aug 9, 2016

Uh oh!

kunalspathak commented Aug 10, 2016

Uh oh!

kunalspathak commented Aug 10, 2016

Uh oh!

pleath commented Aug 10, 2016

Uh oh!

kunalspathak commented Aug 10, 2016

Uh oh!

Uh oh!

kunalspathak commented Aug 2, 2016 •

edited

Loading

kunalspathak commented Aug 2, 2016 •

edited

Loading

agarwal-sandeep commented Aug 5, 2016 •

edited by kunalspathak

Loading

agarwal-sandeep Aug 5, 2016 •

edited by kunalspathak

Loading