Fix memory leak around inlineCache invalidationList #1554
Merged
Conversation
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
|
@pleath, please review. |
| @@ -513,10 +513,6 @@ namespace Js | |||
| template<bool CheckLocal, bool CheckProto, bool CheckAccessor> | |||
| void CloneInlineCacheToEmptySlotInCollision(Type *const type, uint index); | |||
|
|
|||
| #ifdef CLONE_INLINECACHE_TO_EMPTYSLOT | |||
There was a problem hiding this comment.
CLONE_INLINECACHE_TO_EMPTYSLOT [](start = 7, length = 30)
Please dont delete this code. There has been an instance where we ended up using some code under this ifdef. The code under this ifdef is still good to keep around for experimenting with it.
There was a problem hiding this comment.
Alright. Thanks for letting me know.
When inlineCaches are created, they are registered in threadContext in an invalidation list and maintained using registeredInlineCacheCount in threadContext. When these caches are removed from invalidation list, we record that number as well using unregisteredInlineCacheCount in threadContext. If the ratio of registeredInlineCacheCount to unregisteredInlineCacheCount is less than 4, we perform compaction of the invalidation list by deleting unregisteredInlineCacheCount nodes from the list that doesn't hold inlineCache and return their memory back to the arena. Ideally, unregisteredInlineCacheCount should always match no. of inlineCaches that were removed from invalidation list (aka cachesRemoved). However today, cachesRemoved > unregisteredInlineCacheCount most of the time. Because of this, we were not returning cachesRemoved - unregisteredInlineCacheCount nodes of invalidation list back to arena and the memory taken up by these nodes kept piling leading to memory leak. The reason for cachesRemoved > unregisteredInlineCacheCount was because there were couple of places where we were not recording the removal of inlineCaches in unregisteredInlineCacheCount. Also, we didn't update unregisteredInlineCacheCount when we bulk deleted the invalidation list. Fixing these 2 places, the cachesRemoved == unregisteredInlineCacheCount holds true. registeredInlineCacheCount was updated (reduced that count) when we bulk delete entire invalidation lists. However we never updated (again reduced that count) when we performed compaction of invalidation list. Because of this, registeredInlineCacheCount kept growing and it became harder and harder to meet the condition of compaction. This leads to nodes having invalid inline cache still holding memory causing leak. Also did minor code cleanup and added asserts.
kunalspathak
force-pushed
the
memoryleak
branch
from
b6eac75 to
dc43338
Compare
chakrabot
pushed a commit
that referenced
this pull request
Sep 12, 2016
…dationList Merge pull request #1554 from kunalspathak:memoryleak When inlineCaches are created, they are registered in `threadContext` in an invalidation list and maintained using `registeredInlineCacheCount` in `threadContext`. When these caches are removed from invalidation list, we record that number as well using `unregisteredInlineCacheCount` in `threadContext`. If the ratio of `registeredInlineCacheCount` to `unregisteredInlineCacheCount` is less than 4, we perform compaction of the invalidation list by deleting `unregisteredInlineCacheCount` nodes from the list that doesn't hold inlineCache and return their memory back to the arena. * Ideally, `unregisteredInlineCacheCount` should always match no. of inlineCaches that were removed from invalidation list (aka `cachesRemoved`). However today, `cachesRemoved > unregisteredInlineCacheCount` most of the time. Because of this, we were not returning `cachesRemoved - unregisteredInlineCacheCount` nodes of invalidation list back to arena and the memory taken up by these nodes kept piling leading to memory leak. The reason for `cachesRemoved > unregisteredInlineCacheCount` was because there were couple of places where we were not recording the removal of inlineCaches in `unregisteredInlineCacheCount`. Also, we didn't update `unregisteredInlineCacheCount` when we bulk deleted the invalidation list. Fixing these 2 places, the `cachesRemoved == unregisteredInlineCacheCount` holds true. * `registeredInlineCacheCount` was updated (reduced that count) when we bulk delete entire invalidation lists. However we never updated (again reduced that count) when we performed compaction of invalidation list. Because of this, `registeredInlineCacheCount` kept growing and it became harder and harder to meet the condition of compaction. This leads to nodes having invalid inline cache still holding memory causing leak. Also did minor code cleanup and added asserts.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When inlineCaches are created, they are registered in
threadContextin an invalidation list and maintained usingregisteredInlineCacheCountinthreadContext. When these caches are removed from invalidation list, we record that number as well usingunregisteredInlineCacheCountinthreadContext. If the ratio ofregisteredInlineCacheCounttounregisteredInlineCacheCountis less than 4, we perform compaction of the invalidation list by deletingunregisteredInlineCacheCountnodes from the list that doesn't hold inlineCache and return their memory back to the arena.unregisteredInlineCacheCountshould always match no. of inlineCaches that were removed from invalidation list (akacachesRemoved). However today,cachesRemoved > unregisteredInlineCacheCountmost of the time. Because of this, we were not returningcachesRemoved - unregisteredInlineCacheCountnodes of invalidation list back to arena and the memory taken up by these nodes kept piling leading to memory leak. The reason forcachesRemoved > unregisteredInlineCacheCountwas because there were couple of places where we were not recording the removal of inlineCaches inunregisteredInlineCacheCount. Also, we didn't updateunregisteredInlineCacheCountwhen we bulk deleted the invalidation list. Fixing these 2 places, thecachesRemoved == unregisteredInlineCacheCountholds true.registeredInlineCacheCountwas updated (reduced that count) when we bulk delete entire invalidation lists. However we never updated (again reduced that count) when we performed compaction of invalidation list. Because of this,registeredInlineCacheCountkept growing and it became harder and harder to meet the condition of compaction. This leads to nodes having invalid inline cache still holding memory causing leak.Also did minor code cleanup and added asserts.