EntityTree: only check for entity deletions when necessary

[teh-cmc]

• Fixes Chunk ingestion performance regression because of compaction logic #8101

Before:

After:

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):
  • Using examples from latest main build: rerun.io/viewer
  • Using full set of examples from nightly build: rerun.io/viewer
• The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
• If applicable, add a new check to the release checklist!
• If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

• PR Build Summary
• Recent benchmark results
• Wasm size tracking

To run all checks from main, comment on the PR with @rerun-bot full-check.

[jleibs]

I'm not totally following how this improves performance.

The inclusion of entity_paths_with_deletions doesn't change the fact that on_store_deletions does an entire tree-walk.

Is the whole point of this optimization to bypass the overhead of the is_empty() call in cases where we know that the intermediate child couldn't have been deleted?

That said, this seems like this will no longer successfully delete intermediate children that only existed as containers for other entities but don't have their own data.

[teh-cmc]

The inclusion of entity_paths_with_deletions doesn't change the fact that on_store_deletions does an entire tree-walk.

Is the whole point of this optimization to bypass the overhead of the is_empty() call in cases where we know that the intermediate child couldn't have been deleted?

Yeah, the tree walk in itself is imperceptible (it's just a few thousands recursions in the worst case, it's barely measurable) -- a few thousands is_empty() on the other hand is extremely costly.

This PR basically brings the latency down from several seconds (ever increasing) to a constant 10ms (using the benchmark script in the issue).

That said, this seems like this will no longer successfully delete intermediate children that only existed as containers for other entities but don't have their own data.

Haa! I didn't even know that that was the point of this thing. We need to tweak this slightly then.

[jleibs]

a few thousands is_empty() on the other hand is extremely costly

Got it -- that wasn't obvious at all and definitely warrants a comment then. I wonder if is_empty() should be renamed to something like check_if_empty() to better imply there's an active cost to be paid and it's not just accessing some pre-computed state.