onflow / atree

Atree provides scalable arrays and scalable ordered maps.
https://onflow.org
Apache License 2.0
40 stars 16 forks source link

Goroutine creation in FastCommit() #267

Open SaveTheRbtz opened 2 years ago

SaveTheRbtz commented 2 years ago

Issue To Be Solved

Currently, PersistentSlabStorage.FastCommit() does create adhoc goroutines, which may add a substantial overhead on servers with a high number of cores.

Suggested Solution

In the ideal world, each slab storage instance can have a goroutine pool of encoders ready to encode a set of keys. As a bonus, it would also be possible to reuse encoders, instead of creating a new one each time.

fxamacker commented 2 years ago

@SaveTheRbtz Thanks for opening this issue and also PR #268.

Suggested Solution In the ideal world, each slab storage instance can have a goroutine pool of encoders ready to encode a set of keys. As a bonus, it would also be possible to reuse encoders, instead of creating a new one each time.

I think each slab storage instance isn't reused for multiple transaction/script executions.

Unless I'm mistaken or things changed:

For these reasons, I don't think there's much impact or ROI to be gained by adding a pool to each slab storage instance.

Maybe we can evaluate adding a global worker pool of goroutines to atree.

Thoughts? @SaveTheRbtz @turbolent @ramtinms

SaveTheRbtz commented 2 years ago

Good point about slab storage lifetime (should've checked it before suggesting a solution). If cached deltas are common, then we should consider adding an injectable worker pool to atree. I'll take a look at the execution node profiles and come back here with more info.

turbolent commented 2 years ago

Good idea!

Yes, Faye's points describing the current usage of atree in Cadence are still correct.

But it might still make sense to have a worker pool here, either by defining it in atree for all users; or allowing it to be passed in by users, allowing it to be reused (better)