Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Caching: migrate HybridCache api surface from asp.net into runtime #103103

Merged
merged 13 commits into from
Jul 22, 2024
Merged

Caching: migrate HybridCache api surface from asp.net into runtime #103103

merged 13 commits into from
Jul 22, 2024

Conversation

mgravell
Copy link
Member

@mgravell mgravell commented Jun 5, 2024

This is a draft implementation of #100290

@dotnet-issue-labeler dotnet-issue-labeler
Copy link

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.

1 similar comment
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

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.

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-caching
See info in area-owners.md if you want to be subscribed.

@mgravell mgravell mentioned this pull request Jun 5, 2024
Closed
This was referenced Jun 6, 2024
Closed
Closed
@Tornhoof
Copy link
Contributor

Tornhoof commented Jun 6, 2024

With all the nifty allows ref struct stuff, we still don't get non-string keys do we?

@mgravell
Copy link
Member Author

mgravell commented Jun 6, 2024 •
edited
Loading

To offer non-string keys, we'd need the API to take a object key and a Func<object, string> keyFormatter (or a TKey key, but note that since IMemoryCache uses object key, this would box immediately). Not impossible, but makes things quite a bit more complex for the caller. A suitable TKey/TValue L1 exists in a proposal, but has not made it past that (aka RCache).

@build-analysis build-analysis bot mentioned this pull request Jun 13, 2024
Closed
@mgravell
Copy link
Member Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@build-analysis build-analysis bot mentioned this pull request Jun 20, 2024
Open
@mgravell
Copy link
Member Author

mgravell commented Jul 10, 2024 •
edited
Loading

test fail seemingly unrelated:

eng/testing/linker/trimmingTests.targets(168,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) Error: [Failed Test]: /Users/runner/work/1/s/src/libraries/System.Net.Http/tests/TrimmingTests/HttpClientTest.cs. The Command /Users/runner/work/1/s/artifacts/bin/trimmingTests/projects/System.Net.Http.TrimmingTests/HttpClientTest/osx-x64/bin/Release/net9.0/osx-x64/publish/project return a non-success exit code 134.

will retry, and if that doesn't help: try a main merge and rebase

@mgravell
Copy link
Member Author

mgravell commented Jul 10, 2024 •
edited
Loading

(after rebase) shakes fist at CI:

.packages/microsoft.dotnet.helix.sdk/9.0.0-beta.24327.1/tools/Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(91,5): error : (NETCORE_ENGINEERING_TELEMETRY=Test) Work item System.Net.HttpListener.Tests in job 340936c4-63c1-4bac-b738-99505bee54ff has failed.

This was referenced Jul 10, 2024
Open
Closed
Closed
jozkee
jozkee reviewed Jul 12, 2024
View reviewed changes
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

Could you please include tests.

src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hybrid/HybridCache.cs Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hybrid/HybridCache.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hybrid/HybridCache.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hybrid/HybridCache.cs Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hybrid/HybridCache.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hybrid/HybridCache.cs Outdated Show resolved Hide resolved
@build-analysis build-analysis bot mentioned this pull request Jul 18, 2024
Open
stephentoub
stephentoub reviewed Jul 22, 2024
View reviewed changes
src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hybrid/HybridCache.cs
/// <param name="cancellationToken">The <see cref="CancellationToken"/> used to propagate notifications that the operation should be canceled.</param>
/// <returns>The data, either from cache or the underlying data service.</returns>
[System.Diagnostics.CodeAnalysis.SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "Delegate differences make this unambiguous")]
public abstract ValueTask<T> GetOrCreateAsync<TState, T>(string key, TState state, Func<TState, CancellationToken, ValueTask<T>> factory,
Copy link
Member

Choose a reason for hiding this comment

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

Should the order of TState / T be changed so that in all overloads the T is first?

stephentoub
stephentoub reviewed Jul 22, 2024
View reviewed changes
src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hybrid/HybridCache.cs
{
// for the simple usage scenario (no TState), pack the original callback as the "state", and use a wrapper function that just unrolls and invokes from the state
public static readonly Func<Func<CancellationToken, ValueTask<T>>, CancellationToken, ValueTask<T>> Instance = static (callback, ct) => callback(ct);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually necessary? I seem to remember the C# compiler folding identical lambdas into the same method definition (but maybe I'm misremembering?)

stephentoub
stephentoub reviewed Jul 22, 2024
View reviewed changes
src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hybrid/HybridCache.cs
await @this.RemoveAsync(key, cancellationToken).ConfigureAwait(false);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Any tests to add in this repo for the functionality that's here?

@mgravell
Copy link
Member Author

mgravell commented Jul 22, 2024 •
edited
Loading

woohoo!

does anyone in runtime (@stephentoub perhaps) have insight into the CI failure? this doesn't occur locally:

./build.cmd clr+libs -rc Release
...blah
Build succeeded.
0 Warning(s)
0 Error(s)

CI failure:

azure-pipelines
/ runtime (Build windows-x86 release CoreCLR_Libraries)
.dotnet\sdk\9.0.100-preview.5.24307.3\NuGet.RestoreEx.targets#L19
.dotnet\sdk\9.0.100-preview.5.24307.3\NuGet.RestoreEx.targets(19,5): error : (NETCORE_ENGINEERING_TELEMETRY=Restore)
Static graph-based restore failed with exit code '1' but did not log an error.

mgravell and others added 3 commits July 22, 2024 16:44
@mgravell @stephentoub
Update src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hy…
e936a64 
…brid/IHybridCacheSerializer.cs

Co-authored-by: Stephen Toub <[email protected]>
@mgravell @stephentoub
Update src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hy…
7a7bfd5 
…brid/HybridCacheEntryOptions.cs

Co-authored-by: Stephen Toub <[email protected]>
@mgravell @stephentoub
Update src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hy…
d2f8095 
…brid/HybridCacheEntryOptions.cs

Co-authored-by: Stephen Toub <[email protected]>
@stephentoub
Copy link
Member

does anyone in runtime (@stephentoub perhaps) have insight into the CI failure?

Do you have a link to the failure information?

@mgravell
Copy link
Member Author

does anyone in runtime (@stephentoub perhaps) have insight into the CI failure?

Do you have a link to the failure information?

most of the red crosses above; example: https://github.com/dotnet/runtime/runs/27757293802

@stephentoub
Copy link
Member

I'd rack it up to infrastructure hiccups unless it happens again in this next run.

This was referenced Jul 22, 2024
Open
Closed
Closed
@jozkee jozkee merged commit ea9d53e into dotnet:main Jul 22, 2024
80 of 84 checks passed
This was referenced Jul 23, 2024
Closed
Merged
@ericstj ericstj mentioned this pull request Jul 24, 2024
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jozkee jozkee jozkee left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees

@mgravell mgravell

Labels
area-Extensions-Caching new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mgravell @Tornhoof @stephentoub @jozkee