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

Implement pre_load/ post_loadfor the views. #2213

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MathieuDutSik
Copy link
Contributor

@MathieuDutSik MathieuDutSik commented Jul 3, 2024

Motivation

The loading of the views leads to a flurry of independent read_value_bytes which strain the storage. This PR groups the access into a single query.

Proposal

The following was done:

  • The pre_load and post_load are introduced.
  • The NUM_INIT_KEYS static variable is introduced because so far the loading leads to a constant number of reads per view.
  • The fn load was changed to use the pre_load / post_load.

Next steps:

  • Possibly put the fn load as a trait function. But we have now 3 kinds of such load functions so maybe the unification is not a good idea.
  • Change the try_load_entries_mut of reentrant_collection_view so that it uses the pre_load / post_load.

Test Plan

The CI of the view is already challenging enough and do not need to be expanded.
How much of an improvement has been computed:

  • With the old code, the loading of views takes between 45 and 64 milliseconds
  • With the pre_load/post_load, the loading of views takes between 5 and 20 milliseconds.

Release Plan

This should be documented as it is an API change.

Links

Copy link
Contributor

@ma2bd ma2bd left a comment

Choose a reason for hiding this comment

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

Nice. This is going to require a serious review but looks promising.

Also, we kinda assume that read_multi_values_bytes doesn't overflow. Perhaps something to track in a TODO/task.

Comment on lines +176 to +177
let hash = from_bytes_opt(values.first().unwrap())?;
let total_size = from_bytes_opt(values.get(1).unwrap())?.unwrap_or_default();
Copy link
Contributor

@ma2bd ma2bd Jul 14, 2024

Choose a reason for hiding this comment

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

nit: from_bytes_opt could be called from_bytes_option or from_optional_bytes.

More importantly, there should probably be a function for the pattern from_bytes_opt( ... )?.unwrap_or_default(). Maybe from_optional_bytes_{with,or}_default

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

Successfully merging this pull request may close these issues.

None yet

2 participants