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

Primary cache: don't denormalize defaulted component values #4891

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Jan 23, 2024

The cache will now keep track of missing optional components, and store an empty slice instead of a bunch of None values.
When queried, an empty shows up as a None option to the end-user, who can act appropriately.

SFM before:
image

SFM after:
image


What

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@teh-cmc teh-cmc added 🔍 re_query affects re_query itself 📉 performance Optimization, memory use, etc do-not-merge Do not merge this PR exclude from changelog PRs with this won't show up in CHANGELOG.md labels Jan 23, 2024
@teh-cmc teh-cmc linked an issue Jan 23, 2024 that may be closed by this pull request
Copy link

Size changes

Name cmc/primcache_19_statification 4891/merge Change
JS 139.41 kiB 111.52 kiB -20.01%

@teh-cmc teh-cmc force-pushed the cmc/primcache_19_statification branch from dd80167 to 5b4e1d4 Compare January 23, 2024 16:51
Base automatically changed from cmc/primcache_19_statification to main January 23, 2024 17:01
@teh-cmc teh-cmc force-pushed the cmc/primcache_default_denomr branch from 5bf921d to bf89c30 Compare January 23, 2024 17:14
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Jan 24, 2024
@@ -770,6 +770,7 @@ impl CacheBucket {
Ok(added_size_bytes)
}

/// This will insert an empty slice for a missing component (instead of N `None` values).
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to insert/store anything? Can't we just return an empty container from the cache if the component-entry is missing when we try to access it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to keep track of the empty rows so all the component caches can share the same top-level offsets (similar to what we do in the store where we use a Option::<DataCell>::None value for that).

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, for the case of range-queries where only some of the rows are empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

@teh-cmc teh-cmc merged commit ce3ab5f into main Jan 25, 2024
40 of 41 checks passed
@teh-cmc teh-cmc deleted the cmc/primcache_default_denomr branch January 25, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md 📉 performance Optimization, memory use, etc 🔍 re_query affects re_query itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Primary cache: don't denormalize defaulted values
2 participants