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

Remove Gen2 GC based trigger for compacting memory cache #221 #265

Closed
wants to merge 1 commit into from
Closed

Remove Gen2 GC based trigger for compacting memory cache #221 #265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Microsoft.Extensions.Caching.Abstractions/IMemoryCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,11 @@ public interface IMemoryCache : IDisposable
/// </summary>
/// <param name="key">An object identifying the entry.</param>
void Remove(object key);

/// <summary>
/// Attempt to remove the given percentage of the total entries in the cache.
/// </summary>
/// <param name="percentage">The percentage of cache entries to remove.</param>
void Compact(double percentage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the doc comment in MemoryCache itself to be a proper doc comment? Right now there are some odd /// comments there but not an actual doc comment.

Caching/src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

Lines 275 to 281 in cd57aea

/// Remove at least the given percentage (0.10 for 10%) of the total entries (or estimated memory?), according to the following policy:
/// 1. Remove all expired items.
/// 2. Bucket by CacheItemPriority.
/// 3. Least recently used objects.
/// ?. Items with the soonest absolute expiration.
/// ?. Items with the soonest sliding expiration.
/// ?. Larger objects - estimated by object graph size, inaccurate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the implementation in MemoryCache doesn't do any argument validation, so that needs to be fixed as well.

Copy link
Member

@Tratcher Tratcher Jan 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this return the number of elements removed? or a bool indicating if any elements were removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the argument is in percentage, it would probably make more sense to return the percentage of elements removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't seem very informative. Also, percentages are problematic if you remove 0/0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since we are exposing Compact, should we expose Count on the interface as well? It's not a good way to gauge cache size but I suspect users will downcast and rely on it to determine how much to compact like in #252.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of using percentages is that they don't need to know the size. Otherwise Compact would take a specific amount.

Or we shift it so Compact takes a specific number of elements to purge, returns the actual number removed, and expose Count.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then let's keep using percentages for both the amount to remove and amount removed. In the 0/0 case, just return 0. Returning the number of entry removed is rather strange considering a percentage was passed in. Returning a bool is also very uninformative.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eilon except we've punted the decide-when-to-compact duty to the developer, and the number of elements in the cache is one inputs you'd use to decide when and how much to purge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JunTaoLuo put this on hold until we can sync in person Monday.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will probably solicit feedback from @davidfowl and @DamianEdwards as well if possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anyone can ever reason about how much to clear based on the number of items in the cache. Remember, the cache is generally a shared resource. Someone (including our own codes) might have stashed a thousand small objects there. Or 1 really ginormous one.

But yes, let's discuss.

}
}
8 changes: 8 additions & 0 deletions src/Microsoft.Extensions.Caching.Abstractions/exceptions.net45.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[
{
"OldTypeId": "public interface Microsoft.Extensions.Caching.Memory.IMemoryCache : System.IDisposable",
"NewTypeId": "public interface Microsoft.Extensions.Caching.Memory.IMemoryCache : System.IDisposable",
"NewMemberId": "System.Void Compact(System.Double percentage)",
"Kind": "Addition"
}
]
8 changes: 8 additions & 0 deletions src/Microsoft.Extensions.Caching.Abstractions/exceptions.netcore.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[
{
"OldTypeId": "public interface Microsoft.Extensions.Caching.Memory.IMemoryCache : System.IDisposable",
"NewTypeId": "public interface Microsoft.Extensions.Caching.Memory.IMemoryCache : System.IDisposable",
"NewMemberId": "System.Void Compact(System.Double percentage)",
"Kind": "Addition"
}
]
53 changes: 0 additions & 53 deletions src/Microsoft.Extensions.Caching.Memory/Infrastructure/GcNotification.cs
View file
Open in desktop

This file was deleted.

18 changes: 0 additions & 18 deletions src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@ public MemoryCache(IOptions<MemoryCacheOptions> optionsAccessor)
_entryExpirationNotification = EntryExpired;

_clock = options.Clock ?? new SystemClock();
if (options.CompactOnMemoryPressure)
{
GcNotification.Register(DoMemoryPreassureCollection, state: null);
}
_expirationScanFrequency = options.ExpirationScanFrequency;
_lastExpirationScan = _clock.UtcNow;
}
Expand Down Expand Up @@ -276,20 +272,6 @@ private static void ScanForExpiredItems(MemoryCache cache)
}
}

/// This is called after a Gen2 garbage collection. We assume this means there was memory pressure.
/// Remove at least 10% of the total entries (or estimated memory?).
private bool DoMemoryPreassureCollection(object state)
{
if (_disposed)
{
return false;
}

Compact(0.10);

return true;
}

/// Remove at least the given percentage (0.10 for 10%) of the total entries (or estimated memory?), according to the following policy:
/// 1. Remove all expired items.
/// 2. Bucket by CacheItemPriority.
Expand Down
2 changes: 0 additions & 2 deletions src/Microsoft.Extensions.Caching.Memory/MemoryCacheOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ public class MemoryCacheOptions : IOptions<MemoryCacheOptions>
{
public ISystemClock Clock { get; set; }

public bool CompactOnMemoryPressure { get; set; } = true;

public TimeSpan ExpirationScanFrequency { get; set; } = TimeSpan.FromMinutes(1);

MemoryCacheOptions IOptions<MemoryCacheOptions>.Value
Expand Down
12 changes: 12 additions & 0 deletions src/Microsoft.Extensions.Caching.Memory/exceptions.net45.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[
{
"OldTypeId": "public class Microsoft.Extensions.Caching.Memory.MemoryCacheOptions : Microsoft.Extensions.Options.IOptions<Microsoft.Extensions.Caching.Memory.MemoryCacheOptions>",
"OldMemberId": "public System.Boolean get_CompactOnMemoryPressure()",
"Kind": "Removal"
},
{
"OldTypeId": "public class Microsoft.Extensions.Caching.Memory.MemoryCacheOptions : Microsoft.Extensions.Options.IOptions<Microsoft.Extensions.Caching.Memory.MemoryCacheOptions>",
"OldMemberId": "public System.Void set_CompactOnMemoryPressure(System.Boolean value)",
"Kind": "Removal"
}
]
12 changes: 12 additions & 0 deletions src/Microsoft.Extensions.Caching.Memory/exceptions.netcore.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[
{
"OldTypeId": "public class Microsoft.Extensions.Caching.Memory.MemoryCacheOptions : Microsoft.Extensions.Options.IOptions<Microsoft.Extensions.Caching.Memory.MemoryCacheOptions>",
"OldMemberId": "public System.Boolean get_CompactOnMemoryPressure()",
"Kind": "Removal"
},
{
"OldTypeId": "public class Microsoft.Extensions.Caching.Memory.MemoryCacheOptions : Microsoft.Extensions.Options.IOptions<Microsoft.Extensions.Caching.Memory.MemoryCacheOptions>",
"OldMemberId": "public System.Void set_CompactOnMemoryPressure(System.Boolean value)",
"Kind": "Removal"
}
]
1 change: 0 additions & 1 deletion test/Microsoft.Extensions.Caching.Memory.Tests/CacheEntryScopeExpirationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ private IMemoryCache CreateCache(ISystemClock clock)
return new MemoryCache(new MemoryCacheOptions()
{
Clock = clock,
CompactOnMemoryPressure = false,
});
}

Expand Down
5 changes: 5 additions & 0 deletions test/Microsoft.Extensions.Caching.Memory.Tests/CacheServiceExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ public ICacheEntry CreateEntry(object key)
{
throw new NotImplementedException();
}

public void Compact(double percentage)
{
throw new NotImplementedException();
}
}

private class TestDistributedCache : IDistributedCache
Expand Down
1 change: 0 additions & 1 deletion test/Microsoft.Extensions.Caching.Memory.Tests/CompactTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ private MemoryCache CreateCache(ISystemClock clock = null)
return new MemoryCache(new MemoryCacheOptions()
{
Clock = clock,
CompactOnMemoryPressure = false,
});
}

Expand Down
64 changes: 0 additions & 64 deletions test/Microsoft.Extensions.Caching.Memory.Tests/GcNotificationTests.cs
View file
Open in desktop

This file was deleted.

5 changes: 1 addition & 4 deletions test/Microsoft.Extensions.Caching.Memory.Tests/MemoryCacheSetAndRemoveTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ public class MemoryCacheSetAndRemoveTests
{
private static IMemoryCache CreateCache()
{
return new MemoryCache(new MemoryCacheOptions()
{
CompactOnMemoryPressure = false,
});
return new MemoryCache(new MemoryCacheOptions());
}

[Fact]
Expand Down
1 change: 0 additions & 1 deletion test/Microsoft.Extensions.Caching.Memory.Tests/TimeExpirationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ private IMemoryCache CreateCache(ISystemClock clock)
return new MemoryCache(new MemoryCacheOptions()
{
Clock = clock,
CompactOnMemoryPressure = false,
});
}

Expand Down
1 change: 0 additions & 1 deletion test/Microsoft.Extensions.Caching.Memory.Tests/TokenExpirationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ private IMemoryCache CreateCache(ISystemClock clock)
return new MemoryCache(new MemoryCacheOptions()
{
Clock = clock,
CompactOnMemoryPressure = false,
});
}

Expand Down