Fix bug in CheckIfTypeIsEquivalent helper #1368
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8197,24 +8197,52 @@ namespace Js | |
| return false; | ||
| } | ||
|
|
||
| if (!guard->IsInvalidatedDuringSweep() && guard->GetType()->GetScriptContext() != type->GetScriptContext()) | ||
| { | ||
| // For valid guard value, can't cache cross-context objects | ||
| return false; | ||
| } | ||
|
|
||
| // CONSIDER : Add stats on how often the cache hits, and simply force bailout if | ||
| // the efficacy is too low. | ||
|
|
||
| EquivalentTypeCache* cache = guard->GetCache(); | ||
| // CONSIDER : Consider emitting o.type == equivTypes[hash(o.type)] in machine code before calling | ||
| // this helper, particularly if we want to handle polymorphism with frequently changing types. | ||
| Assert(EQUIVALENT_TYPE_CACHE_SIZE == 8); | ||
| Type** equivTypes = cache->types; | ||
|
|
||
| Type* refType = equivTypes[0]; | ||
| if (refType == nullptr || refType->GetScriptContext() != type->GetScriptContext()) | ||
| { | ||
| // We could have guard that was invalidated while sweeping and now we have type coming from | ||
| // different scriptContext. Make sure that it matches the scriptContext in cachedTypes. | ||
| // If not, return false because as mentioned above, we don't cache cross-context objects. | ||
| #if DBG | ||
| if (refType == nullptr) | ||
| { | ||
| for (int i = 1;i < EQUIVALENT_TYPE_CACHE_SIZE;i++) | ||
| { | ||
| AssertMsg(equivTypes[i] == nullptr, "In equiv typed caches, if first element is nullptr, all others should be nullptr"); | ||
| } | ||
| } | ||
| #endif | ||
| return false; | ||
| } | ||
|
|
||
| if (type == equivTypes[0] || type == equivTypes[1] || type == equivTypes[2] || type == equivTypes[3] || | ||
| type == equivTypes[4] || type == equivTypes[5] || type == equivTypes[6] || type == equivTypes[7]) | ||
| { | ||
| #if DBG | ||
| if (PHASE_TRACE1(Js::EquivObjTypeSpecPhase)) | ||
| { | ||
| if (guard->WasReincarnated()) | ||
| { | ||
| Output::Print(_u("EquivObjTypeSpec: Guard 0x%p was reincarnated and working now \n"), guard); | ||
| Output::Flush(); | ||
| } | ||
| } | ||
| #endif | ||
| guard->SetType(type); | ||
| return true; | ||
| } | ||
|
|
@@ -8230,12 +8258,6 @@ namespace Js | |
| // same prototype, any of the equivalent fixed properties will match. If any has been overwritten, the | ||
| // corresponding guard would have been invalidated and we would bail out (as above). | ||
|
|
||
| if (cache->IsLoadedFromProto() && type->GetPrototype() != refType->GetPrototype()) | ||
| { | ||
| if (PHASE_TRACE1(Js::EquivObjTypeSpecPhase)) | ||
|
|
@@ -8292,38 +8314,56 @@ namespace Js | |
| return false; | ||
| } | ||
|
|
||
| int emptySlotIndex = -1; | ||
| for (int i = 0;i < EQUIVALENT_TYPE_CACHE_SIZE;i++) | ||
| { | ||
| if (equivTypes[i] == nullptr) | ||
| { | ||
| emptySlotIndex = i; | ||
| break; | ||
| }; | ||
| } | ||
|
|
||
| // We have some empty slots, let us use those first | ||
| if (emptySlotIndex != -1) | ||
| { | ||
| if (PHASE_TRACE1(Js::EquivObjTypeSpecPhase)) | ||
| { | ||
| Output::Print(_u("EquivObjTypeSpec: Saving type in unused slot of equiv types cache. \n")); | ||
| Output::Flush(); | ||
| } | ||
|
There was a problem hiding this comment. Better to use just one line - OUTPUT_TRACE(Js::EquivObjTypeSpecPhase, _u("EquivObjTypeSpec: Saving type in unused slot of equiv types cache. \n")); #WontFix There was a problem hiding this comment. Yeah, i will just follow the consistency here and keep it PHASE_TRACE1 In reply to: 73625408 [](ancestors = 73625408) |
||
| equivTypes[emptySlotIndex] = type; | ||
| } | ||
| else | ||
| { | ||
| // CONSIDER (EquivObjTypeSpec): Invent some form of least recently used eviction scheme. | ||
| uintptr_t index = (reinterpret_cast<uintptr_t>(type) >> 4) & (EQUIVALENT_TYPE_CACHE_SIZE - 1); | ||
|
|
||
| if (cache->nextEvictionVictim == EQUIVALENT_TYPE_CACHE_SIZE) | ||
| { | ||
| __analysis_assume(index < EQUIVALENT_TYPE_CACHE_SIZE); | ||
| // If nextEvictionVictim was never set, set it to next element after index | ||
| cache->nextEvictionVictim = (index + 1) & (EQUIVALENT_TYPE_CACHE_SIZE - 1); | ||
| } | ||
| else | ||
| { | ||
| Assert(cache->nextEvictionVictim < EQUIVALENT_TYPE_CACHE_SIZE); | ||
| __analysis_assume(cache->nextEvictionVictim < EQUIVALENT_TYPE_CACHE_SIZE); | ||
| equivTypes[cache->nextEvictionVictim] = equivTypes[index]; | ||
| // Else, set it to next element after current nextEvictionVictim index | ||
| cache->nextEvictionVictim = (cache->nextEvictionVictim + 1) & (EQUIVALENT_TYPE_CACHE_SIZE - 1); | ||
| } | ||
|
|
||
| if (PHASE_TRACE1(Js::EquivObjTypeSpecPhase)) | ||
| { | ||
| Output::Print(_u("EquivObjTypeSpec: Saving type in used slot of equiv types cache at index = %d. NextEvictionVictim = %d. \n"), index, cache->nextEvictionVictim); | ||
| Output::Flush(); | ||
| } | ||
| Assert(index < EQUIVALENT_TYPE_CACHE_SIZE); | ||
| __analysis_assume(index < EQUIVALENT_TYPE_CACHE_SIZE); | ||
| equivTypes[index] = type; | ||
| } | ||
|
|
||
| // Fixed field checks allow us to assume a specific type ID, but the assumption is only | ||
| // valid if we lock the type. Otherwise, the type ID may change out from under us without | ||
| // evolving the type. | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added more from perf perspective in
CheckIfTypeIsEquivalentwhere first thing that is done isSo 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 fetchequivTypes[0]and that would benullptrand that is when we would have returned.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kunalspathak pointed out the place where we avoid invalidating if there's still a live type.