Fix/fix json stats report #5831
Conversation
| methodStats ??= _currentStats.GetOrAdd(report.Method, _ => new MethodStats()); | ||
| allTimeMethodStats ??= _allTimeStats.GetOrAdd(report.Method, _ => new MethodStats()); | ||
| // we don't want to block RPC calls any longer than required | ||
| return Task.Run(() => |
There was a problem hiding this comment.
Could just use
await Task.Yield();
To flip to the threadpool at this point?
There was a problem hiding this comment.
Don't need to have a lambda sub method in the method; can just use that and make method async rather than Task.Run
There was a problem hiding this comment.
Ok but why would that be better? I don't have async code here (just sync code I want to fire&forget and do in the background), so introducing async code just to do Yield seems odd.
There was a problem hiding this comment.
agree with Luk here
There was a problem hiding this comment.
so introducing async code just to do
Yieldseems odd.
You've already done all that though? So the additional code for the yield is actually less code: 0a821a3 (delta change) + removing the added indentation from Task.Run
There was a problem hiding this comment.
so introducing async code just to do
Yieldseems odd.You've already done all that though? So the additional code for the yield is actually less code: 0a821a3 (delta change) + removing the added indentation from Task.Run
I introduced tasks, but no state machines, no reentrancy. await would be completely artificial.
| methodStats ??= _currentStats.GetOrAdd(report.Method, _ => new MethodStats()); | ||
| allTimeMethodStats ??= _allTimeStats.GetOrAdd(report.Method, _ => new MethodStats()); | ||
| // we don't want to block RPC calls any longer than required | ||
| return Task.Run(() => |
There was a problem hiding this comment.
agree with Luk here
Changes
JsonRpcLocalStats.BuildReportto avoid conflicts between threads_dividerto static variable, avoiding allocating it each time.StringBuilderavoiding allocating it each time.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Testing in app logs