-
-
Notifications
You must be signed in to change notification settings - Fork 456
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
WIP #184 add async AddAsync and RemoveAsync to ICache #249
WIP #184 add async AddAsync and RemoveAsync to ICache #249
Conversation
Task.FromResult should be available in .NET 4.5. Completed Task is just a shortcut QOL feature which returns a cached completed task. We can add our own I guess. One more thing, I'm not sure if we really have to implement two complete code paths for sync and async. Maybe with ValueTask we can have only one? |
Hi @MichaCo,
Updated defined to include net4.5. So only net 4.0 does not have async support. Is it OK? Added GetCacheItemAsync to ICache and added benchmark tests for async methods: On my machine I get following results: AddSingleBenchmarkBenchmarkDotNet=v0.10.8, OS=Windows 10.0.17134
Processor=Intel Core i7-7700K CPU 4.20GHz (Kaby Lake), ProcessorCount=8
Frequency=10000000 Hz, Resolution=100.0000 ns, Timer=UNKNOWN
[Host] : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.3132.0
Job-VUBPRE : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.3132.0
Runtime=Clr LaunchCount=1 TargetCount=10
WarmupCount=4
GetSingleBenchmarkBenchmarkDotNet=v0.10.8, OS=Windows 10.0.17134
Processor=Intel Core i7-7700K CPU 4.20GHz (Kaby Lake), ProcessorCount=8
Frequency=10000000 Hz, Resolution=100.0000 ns, Timer=UNKNOWN
[Host] : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.3132.0
Job-VUBPRE : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.3132.0
Runtime=Clr LaunchCount=1 TargetCount=10
WarmupCount=4
For distributed cached I don't see much difference between sync and async version, but for Dictionary it is 2 times slower. It is probably because of async/await overhead. |
Yes its find to skip .NET40, I'll probably drop support for that soon anyways. Thanks for doing some performance testing, some interesting results actually. Haven't looked at your code changes yet. Will review it soon |
I will try to replace Task to ValueTask and will run the benchmark again. Will post my results here. |
I created a separate branch and changed all Task to ValueTask. See benchmarks with ValueTask below: AddSingleBenchmarkBenchmarkDotNet=v0.10.8, OS=Windows 10.0.17134
Processor=Intel Core i7-7700K CPU 4.20GHz (Kaby Lake), ProcessorCount=8
Frequency=10000000 Hz, Resolution=100.0000 ns, Timer=UNKNOWN
[Host] : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.3132.0
Job-VUBPRE : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.3132.0
Runtime=Clr LaunchCount=1 TargetCount=10
WarmupCount=4
GetSingleBenchmarkBenchmarkDotNet=v0.10.8, OS=Windows 10.0.17134
Processor=Intel Core i7-7700K CPU 4.20GHz (Kaby Lake), ProcessorCount=8
Frequency=10000000 Hz, Resolution=100.0000 ns, Timer=UNKNOWN
[Host] : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.3132.0
Job-VUBPRE : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.3132.0
Runtime=Clr LaunchCount=1 TargetCount=10
WarmupCount=4
|
looks like we have a winner? ;) |
Yes, ValueTask looks better compared to Task. But sync vs async for inmemory cache still significantly slower. I guess we better not change sync implementation and only add new API for async. |
yup, agree. I might have to do more testing and optimizations to see why exactly it is much slower. |
…andle.GetCacheItem in async method
@MichaCo, |
feel free to create another one, yup Thanks for the great work btw! Hope I get some time soon to merge and review it properly. |
Any updates to this? We have a project that could really benefit from async caching calls... |
@cdonnellytx This might take a while until all this is totally done/done and released. It is a pretty big change and very little time atm |
+1 for this change, async is very important for my organisation |
+1 for this change, async will improve the performance a lot |
+1 we need this as well |
any update on merging this in? seems like it’s been open for over a year. |
+1 for this change. The async/sync code that we need to create to make this work could lead to threadpool starvation, in other words, this is a great library but definitely it needs this :) IMO the additional support for the async code poses minimal risk to the library and the benefits are many. |
+1 for this change ! |
+1, any updates on async support? |
@MichaCo
Attempted to add AddAsync and RemoveAsync methods to ICache. Not sure what to do with .net 4.5 and lower because it does not support Task.CompletedTask and Task.FromResult.
Any advice? Could we add async support only to .netstandard?