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

Fix performance regression when viewing images and tensors #3767

Merged
merged 7 commits into from Oct 10, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented Oct 10, 2023

What

When hovering images the InstanceKey corresponds to the hovered pixel value (tensors are mono-components; their elements are their instances). We accidentally were using this instance key as the key for several of our caches, which means we would get constant cache misses when hovering an image, leading to huge performance degradation.

This PR switches to using RowId as the primary key for tensors. In particular, it is the row id of the TensorData component, so I call it tensor_data_row_id everywhere for consistency and clarity. There can never be more than on TensorData per row, and each row has a unique RowId, so this works well.

This simplifies a lot of the code too, removing most uses of VersionedInstancePath and VersionedInstancePathHash.

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 demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@emilk emilk added 🪳 bug Something isn't working 📉 performance Optimization, memory use, etc 🦟 regression A thing that used to work in an earlier release include in changelog labels Oct 10, 2023
Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

I'm approving for the purposes of fixing the performance issue, but the real bug here in my opinion is:

When hovering images the InstanceKey corresponds to the hovered pixel value (tensors are mono-components; their elements are their instances).

This really feels like an abuse of the definition of instance key which created a foot-gun. InstanceKeys have a very specific meaning with regards to how they should be able to be used in conjunction with queries and changing that meaning on a per-component basis is just asking for trouble in some other generic join / query / filter code in the future.

@emilk
Copy link
Member Author

emilk commented Oct 10, 2023

Yes, it is confusing, but this is in the context of the GPU picking code which uses a hash of the entity path plus an instance key to identify stuff. By reusing the instance key for identifying the pixel in an image, we can read out what pixel the user is hovering exactly, which is pretty useful, but lead to this footgun.

@emilk emilk merged commit 7316aeb into main Oct 10, 2023
34 of 35 checks passed
@emilk emilk deleted the emilk/fix-tensor-caches branch October 10, 2023 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working include in changelog 📉 performance Optimization, memory use, etc 🦟 regression A thing that used to work in an earlier release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Huge performance regression when hovering large images
2 participants