M.E.C.M. Add TryGetValue(ReadOnlySpan<char>) API by mgravell · Pull Request #112695 · dotnet/runtime

mgravell · 2025-02-19T15:32:15Z

Implement MemoryCache.TryGetValue APIs taking ReadOnlySpan<char>, as approved in #110504

ghost · 2025-02-19T15:32:23Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2025-02-19T15:32:24Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copilot

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

- demand GetAlternateLookup works on the target runtimes - rely on documented "out set to default on failure" lookup behavior

mgravell · 2025-02-19T16:04:47Z

@stephentoub made less defensive in 4f60149, per feedback

…into marc/MECMSpanGet

stephentoub · 2025-02-20T15:18:32Z

+        public bool TryGetValue(System.ReadOnlySpan<char> key, out object? value) { throw null; }
+        [System.Runtime.CompilerServices.OverloadResolutionPriority(1)]
+        public bool TryGetValue<TItem>(System.ReadOnlySpan<char> key, out TItem? value) { throw null; }
+#endif


This will get stomped on when anyone runs the ref generator. We prefer to put such TFM-specific methods in a separator file, e.g. https://github.com/dotnet/runtime/blob/d63f63725a6fd71c7b723640a2418d0f7a30e7bc/src/libraries/Microsoft.Extensions.Caching.Abstractions/ref/Microsoft.Extensions.Caching.Abstractions.netcoreapp.cs

stephentoub · 2025-02-20T15:19:58Z


            CoherentState coherentState = _coherentState; // Clear() can update the reference in the meantime
-            if (coherentState.TryGetValue(key, out CacheEntry? tmp))
+            coherentState.TryGetValue(key, out CacheEntry? entry); // note we rely on documented "default when fails" contract re the out


We could formalize the comment with an assert, e.g.

Suggested change

coherentState.TryGetValue(key, out CacheEntry? entry); // note we rely on documented "default when fails" contract re the out

bool success = coherentState.TryGetValue(key, out CacheEntry? entry);

Debug.Assert(success || entry is null, "We rely on documented 'default when fails' contract.");

stephentoub · 2025-02-20T15:21:34Z

+        /// <param name="value">The located value or null.</param>
+        /// <returns>True if the key was found.</returns>
+        /// <remarks>This method allows values with <see cref="string"/> keys to be queried by content without allocating a new <see cref="string"/> instance.</remarks>
+        [OverloadResolutionPriority(1)]


@333fred, @jaredpar, just FYI on more use of ORPA.

stephentoub · 2025-02-20T15:59:28Z

This didn't actually run CI

stephentoub · 2025-02-20T16:00:09Z

It also merged with https://github.com/dotnet/runtime/pull/112695/files#r1963779247 unaddressed

mgravell · 2025-02-20T21:45:52Z

Well shoot, it was all briefly green. Will fix: sorry.

…

On Thu, 20 Feb 2025, 16:00 Stephen Toub, ***@***.***> wrote: It also merged with https://github.com/dotnet/runtime/pull/112695/files#r1963779247 unaddressed — Reply to this email directly, view it on GitHub <#112695 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMEQJOGMXH7NN7MLLBT2QX3Z7AVCNFSM6AAAAABXOPZ4Q6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZRHEZTINBWGY> . You are receiving this because you modified the open/close state.Message ID: ***@***.***> [image: stephentoub]*stephentoub* left a comment (dotnet/runtime#112695) <#112695 (comment)> It also merged with https://github.com/dotnet/runtime/pull/112695/files#r1963779247 unaddressed — Reply to this email directly, view it on GitHub <#112695 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMEQJOGMXH7NN7MLLBT2QX3Z7AVCNFSM6AAAAABXOPZ4Q6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZRHEZTINBWGY> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

M.E.C.M. Add TryGetValue(ReadOnlySpan<char>) API

a481506

fix dotnet#110504

Copilot AI review requested due to automatic review settings February 19, 2025 15:32

ghost added area-Extensions-Caching new-api-needs-documentation labels Feb 19, 2025

dotnet-policy-service Bot assigned mgravell Feb 19, 2025

Copilot AI reviewed Feb 19, 2025

View reviewed changes

fix test glitch

28e7ad0

stephentoub reviewed Feb 19, 2025

View reviewed changes

Comment thread src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs Outdated

stephentoub reviewed Feb 19, 2025

View reviewed changes

Comment thread src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs Outdated

stephentoub reviewed Feb 19, 2025

View reviewed changes

Comment thread src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs Outdated

stephentoub reviewed Feb 19, 2025

View reviewed changes

Comment thread src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs Outdated

PR feedback; be less defensive:

4f60149

- demand GetAlternateLookup works on the target runtimes - rely on documented "out set to default on failure" lookup behavior

Merge branch 'main' into marc/MECMSpanGet

8709fa8

vcsjones reviewed Feb 19, 2025

View reviewed changes

Comment thread src/libraries/Microsoft.Extensions.Caching.Memory/ref/Microsoft.Extensions.Caching.Memory.cs Outdated

Comment thread src/libraries/Microsoft.Extensions.Caching.Memory/ref/Microsoft.Extensions.Caching.Memory.cs Outdated

mgravell added 2 commits February 20, 2025 14:33

address PR feedback

474041d

Merge branch 'marc/MECMSpanGet' of https://github.com/mgravell/runtime …

d63f637

…into marc/MECMSpanGet

stephentoub reviewed Feb 20, 2025

View reviewed changes

Comment thread src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs Outdated

stephentoub reviewed Feb 20, 2025

View reviewed changes

stephentoub approved these changes Feb 20, 2025

View reviewed changes

consolidate "now" lookup into PostProcessTryGetValue

7b32ea2

mgravell merged commit 7e9343b into dotnet:main Feb 20, 2025

mgravell mentioned this pull request Feb 21, 2025

MemoryCache: use net9.cs file rather than #if for TFM limits #112786

Merged

mgravell mentioned this pull request Feb 24, 2025

[API Proposal]: HybridCache: span-based fetch #112866

Closed

LoopedBard3 mentioned this pull request Feb 25, 2025

[Perf] Windows/x64: 6 Regressions on 2/20/2025 3:57:18 PM +00:00 #112914

Closed

This was referenced Mar 13, 2025

[Perf] Linux/x64: 46 Regressions on 2/20/2025 3:57:18 PM +00:00 dotnet/perf-autofiling-issues#51117

Open

[wasm][perf] Tracking #96444

Open

github-actions Bot locked and limited conversation to collaborators Mar 23, 2025

	coherentState.TryGetValue(key, out CacheEntry? entry); // note we rely on documented "default when fails" contract re the out
	bool success = coherentState.TryGetValue(key, out CacheEntry? entry);
	Debug.Assert(success \|\| entry is null, "We rely on documented 'default when fails' contract.");

Uh oh!

Conversation

mgravell commented Feb 19, 2025

Uh oh!

ghost commented Feb 19, 2025

Uh oh!

ghost commented Feb 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgravell commented Feb 19, 2025

Uh oh!

Uh oh!

Uh oh!

stephentoub Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stephentoub Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Feb 20, 2025

Uh oh!

stephentoub commented Feb 20, 2025

Uh oh!

mgravell commented Feb 20, 2025 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants