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

Goroutine creation in FastCommit() #267

Open
SaveTheRbtz opened this issue Jun 3, 2022 · 3 comments
Open

Goroutine creation in FastCommit() #267

SaveTheRbtz opened this issue Jun 3, 2022 · 3 comments
Assignees

Comments

@SaveTheRbtz
Copy link
Contributor

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
Copy link
Member

@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:

  • Cadence runtime creates atree storage for each transaction/script execution.
  • Cadence runtime calls atree storage to write cached deltas at the end of execution.
  • FastCommit() only creates goroutines when there is cached deltas to be committed.

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
Copy link
Contributor Author

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
Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants