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.

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

Reduce contention inside RocksDB during OldReceipts #6890

Merged
merged 4 commits into from
Apr 9, 2024
Merged

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Apr 2, 2024

Resolves #4642

Changes

  • Lock around _blockStore.Get in BlockTree to move the contention outside of the RocksDB allocator (which then heavily hits other RocksDB processing like block processing) and into the calling code (OldReceipts/OldBodies)
  • A better soltion would be to change the allocator to tcmalloc or Jemalloc however that is much more complicated especially factoring cross platform (will need to be building RocksDB from source and compiling in the allocator) Use Jemalloc for significant memory reduction #6107

Before

image

After

image

Types of changes

What types of changes does your code introduce?

  • Optimization

Testing

Requires testing

  • No

Documentation

Requires explanation in Release Notes

  • Yes

Significantly reduces block processing times

Remarks

On Windows was seeing block processing times of up to 24seconds, this brings it down to mostly in range with the occasional spike to 2secs

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

As it is in block tree it may bottleneck other use cases like json rpc - I'm not fan of this workaround, can we try changing allocator?
If not should we make this lock in Receipts Sync only?
In the past we were building rocks db so bringing it back shouldn't be too complicated. @rubo and @matilote should have knowledge about it.

// We take the lock here instead to reduce contention on the allocator in the db.
// A better solution would be to change the allocator https://github.com/NethermindEth/nethermind/issues/6107
using var handle = _allocatorLock.Acquire();

block = _blockStore.Get(
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should be in BlockStore. There is an extension method that also does the deserialization. Probably pass the lock into the method so that the deserialization is not locked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will have allocated and deallocted returning the memory to the pool to be used for next block at this level? So also use less memory and not contend on free?

@asdacap
Copy link
Contributor

asdacap commented Apr 2, 2024

Is this like a windows specific behaviour?

@benaadams
Copy link
Member Author

Is this like a windows specific behaviour?

My traces are locks inside RtlAllocateHeap which is Windows specific, so I assume the behaviour on Linux will be different.

image

@asdacap
Copy link
Contributor

asdacap commented Apr 2, 2024

Is this only during OldBodies/OldReceipts? What about after that?

@benaadams benaadams changed the title Reduce contention inside RocksDB during OldReceipts/OldBodies Reduce contention inside RocksDB during OldReceipts Apr 2, 2024
@benaadams
Copy link
Member Author

Is this only during OldBodies/OldReceipts? What about after that?

Yes, is perfectly fine and back to normal after (with or without additional lock); is more in OldReceipts than OldBodies as OldBodies is just setting and not making large allocations; whereas OldReceipts is pulling all the Bodies out of the db so using the db allocator heavily

@benaadams
Copy link
Member Author

As it is in block tree it may bottleneck other use cases like json rpc

Which should prevent multiple concurrent json rpc requests for blocks bottlenecking block processing also?

@asdacap
Copy link
Contributor

asdacap commented Apr 2, 2024

I assume oldreceipts slow down also?

@benaadams
Copy link
Member Author

I assume oldreceipts slow down also?

I think it was faster (on Windows); will try again.

However if the allocator is actually working better in parallel (e.g. tcmalloc or Jemalloc) then it would be slower since it makes it single thread at this point. Not sure how the default Linux allocator used by RocksDb performs.

@benaadams
Copy link
Member Author

benaadams commented Apr 8, 2024

On Windows

Pre-change 9hrs 24mins for first 56% of receipts

2024-04-08 02:03:11|Old Receipts         68 / 19,592,894 (  0.00 %) | queue     3,421 | current            0 Blk/s | total           14 Blk/s 
2024-04-08 11:27:06|Old Receipts 10,924,099 / 19,592,894 ( 55.76 %) | queue    36,316 | current          913 Blk/s | total          323 Blk/s 

Post-change 56 mins for remaining 42%

2024-04-08 11:31:08|Old Receipts 11,086,303 / 19,592,894 ( 56.58 %) | queue     1,676 | current          414 Blk/s | total          368 Blk/s 
2024-04-08 12:27:14|Old Receipts 19,364,833 / 19,592,894 ( 98.84 %) | queue   228,029 | current           13 Blk/s | total        2,400 Blk/s 

@benaadams benaadams merged commit e77787a into master Apr 9, 2024
67 checks passed
@benaadams benaadams deleted the oldreceipts branch April 9, 2024 13:02
@benaadams benaadams mentioned this pull request Dec 5, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attestations are being more often missed during OldBodies/OldReceipts stage - optimization
3 participants