-
Notifications
You must be signed in to change notification settings - Fork 767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HybridCache : implement the tag expiration feature #5785
base: main
Are you sure you want to change the base?
Conversation
Note Reposting my comment here from there for ease of discussion. Hi Marc, not strictly related to tagging, but I'm looking at some code change in the PR and I spotted this xml comment regarding
When we touched on this subject some time ago I remember we agreed on the fact that inheritance (from per-call up to the All of this was because of the impossibility to express "undefined" or, to better say it, the impossibility to differentiate between A practical example of a problem this may create: in the Can you clarify what is the current rationale here? I'm asking for 2 reasons: first I'm interested in general, and second because as you know I'm creating a 3rd party implementation, and I'd like the observable external behaviour to be the same for both implementations. Thanks! ps: if you like I can open a different issue for this, just let me know. |
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=912260&view=codecoverage-tab |
An extra point to think about, again related the entry options inheritance thing, is how this method (marked internal, and maybe it will not be used in the end) would be affected. |
Shouldn't we be concerned about machine clocks not being synchronized? i.e. Machine 1 says it's 10:05:03 and adds a timeout for 10:10:03, but Machine 2 says it's 10:12:05 and when it reads the cache item it will determine it to be invalid. |
Eh, welcome to the world of clock shifting/drifting/skewing & friends: it's not something that's solvable via software, at least not at this level (to the best of my knowledge). Just to make an example: typically Microsoft/Meta/Google/AWS have their own private atomic clocks around the world to avoid this problem (... as much as possible), usually dedicated to specific services (like Google Spanner with TrueTime). Some interesting reads:
|
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=913450&view=codecoverage-tab |
Clock skew: ultimately as stated, not easily solvable. But in the context
of cache servers: we don't need super complexity or accuracy: IMO if any
layer is unhappy re expiration, that's good enough to consider it invalid.
As a side-effect, it also makes the logic time-testable.
|
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=931777&view=codecoverage-tab |
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=931922&view=codecoverage-tab |
47a4123
to
1b89029
Compare
…into marc/hc-tags3
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=933691&view=codecoverage-tab |
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=934881&view=codecoverage-tab |
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=934939&view=codecoverage-tab |
src/Libraries/Microsoft.Extensions.Caching.Hybrid/Internal/TagSet.cs
Outdated
Show resolved
Hide resolved
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=935018&view=codecoverage-tab |
…Set.cs Co-authored-by: Sébastien Ros <[email protected]>
…into marc/hc-tags3
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=935225&view=codecoverage-tab |
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=938365&view=codecoverage-tab |
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=938418&view=codecoverage-tab |
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.
Approved; but would be happy if someone else takes a look (PR is long living). Below asked a couple of specific code questions; overall purpose of PR is kind of understandable, but I dont have any background with HybridCache
@@ -58,7 +62,7 @@ public byte[] ToArray() | |||
} | |||
|
|||
var copy = new byte[length]; | |||
Buffer.BlockCopy(Array!, 0, copy, 0, length); | |||
Buffer.BlockCopy(OversizedArray!, Offset, copy, 0, length); |
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.
not sure if it is important, but Span.CopyTo
is faster than Buffer.BlockCopy
. See proof
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.
nice; I will integrate that next cycle
var length = HybridCachePayload.Write(oversized, key, cacheItem.CreationTimestamp, GetL2AbsoluteExpirationRelativeToNow(options), | ||
HybridCachePayload.PayloadFlags.None, cacheItem.Tags, payload.AsSequence()); | ||
|
||
await SetDirectL2Async(key, new(oversized, 0, length, true), GetL2DistributedCacheOptions(options), token).ConfigureAwait(false); |
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.
is ConfigureAwait(false)
needed for lower FXs?
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.
for environments that have a sync-context, which usually means lower FXs; however, as library code: we don't know about the sync-context state, so: best to be explicit
I believe the plan to allow a global setting for this is still being discussed; I will check whether that finally shipped!
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.
if this were "aspnetcore code, only targeting netcurrent", then yes: we could probably get away without specifying this; however, HC a: can be used in non-aspnetcore contexts (WinForms, etc), and b: supports netstandard, netfx, etc
|
||
byte[] oversized = ArrayPool<byte>.Shared.Rent(sizeof(long)); | ||
BinaryPrimitives.WriteInt64LittleEndian(oversized, timestamp); | ||
var pending = SetDirectL2Async(TagKeyPrefix + tag, new BufferChunk(oversized, 0, sizeof(long), false), _tagInvalidationEntryOptions, token); |
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.
is there any overhead calling await SetDirectL2Async
right here? If it is an already finished, isn't await doing nothing more than just getting a result?
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.
(i guess the async-state-machine will still be generated, yep?)
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.
yes, this is to avoid the state machine overhead in the happy path; that isn't terribad, but: it is sometimes desirable to hand-crank it
@@ -20,8 +21,11 @@ namespace Microsoft.Extensions.Caching.Hybrid.Internal; | |||
/// <summary> | |||
/// The inbuilt implementation of <see cref="HybridCache"/>, as registered via <see cref="HybridCacheServiceExtensions.AddHybridCache(IServiceCollection)"/>. | |||
/// </summary> | |||
[SkipLocalsInit] |
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.
why SkipLocalsInit
exists on specific types only? Not on module?
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.
I'd be fine with either, but I don't really need it globally; least surprises, etc
(note: this is a re-do of #5748 / #5781 which went "a bit wrong" when rebasing)
Implement tag expiration in
HybridCache
The
HybridCache
system has a "tags" feature; rather than building a bespoke per-backend secondary index (like how the redis OutputCache backend works), here we move everything into the core library:DefaultHybridCache
, we maintain a map of tags (string
, note*
means everything) to expirations, and we track the creation time against eachCacheItem
Because the tag fetches are async, the data in the lookup is not
(timestamp)
, it isTask<(timestamp)>
Outstanding:
Microsoft Reviewers: Open in CodeFlow