Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Remove GC Compaction trigger #221

Closed
muratg opened this issue Aug 22, 2016 · 13 comments
Closed

Remove GC Compaction trigger #221

muratg opened this issue Aug 22, 2016 · 13 comments

Comments

@muratg
Copy link

muratg commented Aug 22, 2016

@KKhurin @davidfowl

Looks like we may have room for improvement here.

@muratg
Copy link
Author

muratg commented Sep 23, 2016

@KKhurin Are you able to help us with this?

@KKhurin
Copy link

KKhurin commented Sep 24, 2016

Let's have a discussion.

@DamianEdwards
Copy link
Member

I just hit what I think is a down side of the current implementation in that it appears we purge the cache on every Gen2 collection, which in my case was occurring during app start as I was forcibly priming my cache.

@JunTaoLuo
Copy link
Contributor

Currently compaction is triggered by a gen2 collection by the GC. This is not the best trigger since it may occur frequently and does not correlate to the actual memory usage of the cache.

The proposal is to remove the current trigger and instead rely on the user or external triggers to decide when to compact the cache. This will require us to expose the Compact on the interface. Note that there's currently no way of accurately determining the memory usage by the cache so it's difficult to figure out if or how much compaction should occur.

@Eilon
Copy link
Member

Eilon commented Jan 20, 2017

@JunTaoLuo can you update the bug title to reflect the decisions that were made (e.g. change Foo to X because Y).

@JunTaoLuo JunTaoLuo changed the title Investigate memory pressure algorithm in the memory cache Remove GC compaction trigger in MemoryCache and expose Compact on IMemoryCache to allow user/external cache compaction triggers Jan 20, 2017
@JunTaoLuo JunTaoLuo changed the title Remove GC compaction trigger in MemoryCache and expose Compact on IMemoryCache to allow user/external cache compaction triggers Remove GC compaction trigger in MemoryCache and expose Compact on IMemoryCache to allow custom/external triggers Jan 20, 2017
@JunTaoLuo JunTaoLuo changed the title Remove GC compaction trigger in MemoryCache and expose Compact on IMemoryCache to allow custom/external triggers Compact MemoryCache by exposing ICompactionStrategy and a trigger on MemoryCacheOptions Mar 1, 2017
@JunTaoLuo JunTaoLuo changed the title Compact MemoryCache by exposing ICompactionStrategy and a trigger on MemoryCacheOptions Remove GC Compaction trigger Mar 27, 2017
@JunTaoLuo
Copy link
Contributor

After the latest discussions, we decided to remove the memory compaction trigger based on Gen2 GC. To compact, downcast to MemoryCache and call Compact when needed.

@JunTaoLuo
Copy link
Contributor

Will file follow up issue for revisiting the memory cache design.

@Eilon
Copy link
Member

Eilon commented Mar 27, 2017

@JunTaoLuo is this change announcement-worthy?

@JunTaoLuo
Copy link
Contributor

Yup that's why there is a breaking changes tag. Will get to it.

@muratg
Copy link
Author

muratg commented Mar 27, 2017

@JunTaoLuo Do you want to keep the bug open in that case?

@JunTaoLuo
Copy link
Contributor

JunTaoLuo commented Mar 28, 2017

I will open a different issue in the Discussion milestone for the discussion to the announcement. This one just tracks the work item.

@code-mogwai
Copy link

Just checking the workaround for version 1.0.0 is to set CompactOnMemoryPressure to false for the MemoryCache instance?

(FYI, what we think we are experiencing as a result of this design bug is poor caching and only 30% memory utilisation on a server with 16GB RAM. Another server with 32GB RAM and the same application is also at 30% memory utilisation.)

@JunTaoLuo
Copy link
Contributor

@code-mogwai yes, if this is affecting you, you should set CompactOnMemoryPressure to false. If you need to compact, you can downcast to MemoryCache and call Compact directly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants