Skip to content

Conversation

@jleibs
Copy link
Member

@jleibs jleibs commented Dec 15, 2023

What

After adding some scopes, found some more wins on query processing. This is a mixture of some optimized short-circuit early-outs, and doing the PropertyOverride logic on a second-pass in order to avoid any overhead related to creating the EntityProperty structures and override entity-paths.

This has the added benefit of also removing the second query and replacing it with a much easier-to-follow override processor.

Before:
image

After:
image

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

@jleibs jleibs requested a review from Wumpf December 15, 2023 22:28
@jleibs jleibs added 📉 performance Optimization, memory use, etc exclude from changelog PRs with this won't show up in CHANGELOG.md labels Dec 15, 2023
@jleibs jleibs marked this pull request as ready for review December 15, 2023 22:30
@Wumpf Wumpf force-pushed the jleibs/skip_empty_queries branch from 3f0cd4a to f3e06ae Compare December 18, 2023 08:47
Base automatically changed from jleibs/skip_empty_queries to main December 18, 2023 09:05
@Wumpf Wumpf force-pushed the jleibs/optmize_queries branch from 80d3841 to 5a0d21e Compare December 18, 2023 09:05
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

I think the code needs to clear up better what's fast vs normal. Looks & feels very hacky right now. Let's maybe talk it through a bit

/// Called at the start of each frame
pub fn on_frame_start(&mut self, item_retain_condition: impl Fn(&Item) -> bool) {
re_tracing::profile_function!();
re_tracing::profile_scope!("SelectionState::on_frame_start");
Copy link
Member

Choose a reason for hiding this comment

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

isn't that esactly what the profile_function macro prints, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite -- it seems like puffin doesn't track the struct name and so I was seeing two calls to on_frame_start coming from different call-sites resulting in some unexpected aggregation.

Copy link
Member

@Wumpf Wumpf Dec 21, 2023

Choose a reason for hiding this comment

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

Is that with latest puffin? afaik what scopes are merged when improved a bit, but might have been mostly about traits

@jleibs jleibs force-pushed the jleibs/optmize_queries branch 2 times, most recently from 0329e12 to 18571c5 Compare December 21, 2023 01:45
@jleibs jleibs force-pushed the jleibs/optmize_queries branch from 18571c5 to b822d31 Compare December 21, 2023 01:48
@jleibs jleibs changed the title Optimization pass on query execution Speed up query usage in heuristics by splitting DataQuery into separate query and update_properties passes Dec 21, 2023
@jleibs jleibs requested a review from Wumpf December 21, 2023 01:49
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

so much better, flow is totally clear now! The only wrinkle now is the thing you already put in the comments of having the same query results object with and without filled out properties

// in on_frame_start, but on_frame_start also needs the queries.
let updated_query_results = {
{
re_tracing::profile_scope!("updated_query_results");
Copy link
Member

Choose a reason for hiding this comment

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

nit: more like "update_overrides" now

@Wumpf Wumpf merged commit acf0e78 into main Dec 21, 2023
@Wumpf Wumpf deleted the jleibs/optmize_queries branch December 21, 2023 09:19
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix performance regressions for heuristics

3 participants