Let caches clear and code commit in background#9012
Let caches clear and code commit in background#9012
Conversation
benaadams
requested review from
LukaszRozmej,
MarekM25,
asdacap and
smartprogrammer93
benaadams
force-pushed
the
background-caches
branch
5 times, most recently
from
6eb00b8 to
34a43ac
Compare
benaadams
changed the title
benaadams
force-pushed
the
background-caches
branch
from
1355a96 to
1478a28
Compare
MarekM25
left a comment
MarekM25
left a comment
There was a problem hiding this comment.
We should monitor the performance of all background tasks. In some use cases, their performance might be more critical than that of non-background tasks
| Update(); | ||
| } | ||
| catch (ObjectDisposedException od) when (od.ObjectName == "Nethermind.Db.Rocks.DbOnTheRocks") | ||
| { |
There was a problem hiding this comment.
Should we log this at least? It might be confusing if happens on production code
There was a problem hiding this comment.
Again, this is not right, either have a test mode, or better expose something test can await before teardown
| Task codeFlushTask = !commitRoots || _codeBatch is null || _codeBatch.Count == 0 | ||
| ? Task.CompletedTask | ||
| : CommitCodeAsync(); | ||
| if (commitRoots && _codeBatch?.Count > 0) |
There was a problem hiding this comment.
Quick Question: Can it cause consensus issues if a few commits are made before the previous async write has finished?
We're doing multiple commits during block processing, and we might encounter a situation where the cache hasn't been fully cleared or the previous commit hasn't completed, yet we begin processing a new block.
| } | ||
| catch (ObjectDisposedException) | ||
| { | ||
| // Ignore, disposed object pool policy (cancelled in test DI) |
There was a problem hiding this comment.
This should be only in test context? Or maybe better we can expose something that will be awaited in tests, so this doesn't happen
| } | ||
| catch (ObjectDisposedException) | ||
| { | ||
| // Ignore, disposed object pool policy (cancelled in test DI) |
There was a problem hiding this comment.
This should be only in test context? Or maybe better we can expose something that will be awaited in tests, so this doesn't happen
|
|
||
| for (int i = 0; i < threads - 1; i++) | ||
| // Queue work items to the thread pool for all threads except the current one | ||
| var tasks = new ParallelUnbalancedWork[threads - 1]; |
|
|
||
| // Queue work items to the thread pool for all threads except the current one | ||
| for (int i = 0; i < threads - 1; i++) | ||
| var tasks = new InitProcessor<TLocal>[threads - 1]; |
| Update(); | ||
| } | ||
| catch (ObjectDisposedException od) when (od.ObjectName == "Nethermind.Db.Rocks.DbOnTheRocks") | ||
| { |
There was a problem hiding this comment.
Again, this is not right, either have a test mode, or better expose something test can await before teardown
smartprogrammer93
left a comment
smartprogrammer93
left a comment
There was a problem hiding this comment.
Time to revive this. Looks good
Changes
We currently wait for the prewarm caches/processing to clear before completing the block processing as an over abundance of caution when we introduced the feature. We also wait at start of next block.
Before
After
The sync-over-async
.GetAwaiter().GetResult()will occasionally get hit at the end of block and it is quite harsh for performance; so just remove it (and will be taken care of before following block, when is more likely to be complete)Clear caches:
Write code sync:
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?