From 07c908a426a2da339e32e4521a00f04610fb5803 Mon Sep 17 00:00:00 2001 From: Olivier FAURE Date: Sun, 25 Aug 2024 10:51:02 +0200 Subject: [PATCH] Remove bloom filter (#536) The filter was only used for RouteFocusChanged which is about to be removed. Since it was a performance feature, we can remove it early. --- Cargo.lock | 1 - Cargo.toml | 1 - masonry/Cargo.toml | 1 - masonry/src/bloom.rs | 157 -------------------- masonry/src/contexts.rs | 12 -- masonry/src/lib.rs | 1 - masonry/src/widget/tests/lifecycle_basic.rs | 28 ---- masonry/src/widget/widget_pod.rs | 22 +-- masonry/src/widget/widget_ref.rs | 10 -- masonry/src/widget/widget_state.rs | 3 - 10 files changed, 4 insertions(+), 232 deletions(-) delete mode 100644 masonry/src/bloom.rs diff --git a/Cargo.lock b/Cargo.lock index fda13006..73335c40 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1883,7 +1883,6 @@ dependencies = [ "cursor-icon", "dpi", "float-cmp", - "fnv", "futures-intrusive", "image", "insta", diff --git a/Cargo.toml b/Cargo.toml index e210aaf6..986da26d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -115,7 +115,6 @@ winit = "0.30.4" tracing = { version = "0.1.40", default-features = false } smallvec = "1.13.2" dpi = "0.1.1" -fnv = "1.0.7" image = { version = "0.25.2", default-features = false } web-time = "1.1.0" bitflags = "2.6.0" diff --git a/masonry/Cargo.toml b/masonry/Cargo.toml index b7be1774..746ff267 100644 --- a/masonry/Cargo.toml +++ b/masonry/Cargo.toml @@ -26,7 +26,6 @@ parley.workspace = true winit.workspace = true smallvec.workspace = true tracing = { workspace = true, features = ["default"] } -fnv.workspace = true image.workspace = true once_cell = "1.19.0" serde = { version = "1.0.204", features = ["derive"] } diff --git a/masonry/src/bloom.rs b/masonry/src/bloom.rs deleted file mode 100644 index 67f7a79f..00000000 --- a/masonry/src/bloom.rs +++ /dev/null @@ -1,157 +0,0 @@ -// Copyright 2020 the Xilem Authors and the Druid Authors -// SPDX-License-Identifier: Apache-2.0 - -//! A simple Bloom filter, used to track child widgets. - -use std::hash::{Hash, Hasher}; -use std::marker::PhantomData; - -// TODO - Once the bloom filter goes away, remove the fnv dependency. -use fnv::FnvHasher; - -const NUM_BITS: u64 = 64; - -// the 'offset_basis' for the fnv-1a hash algorithm. -// see http://www.isthe.com/chongo/tech/comp/fnv/index.html#FNV-param -// -// The first of these is the one described in the algorithm, the second is random. -const OFFSET_ONE: u64 = 0xcbf2_9ce4_8422_2325; -const OFFSET_TWO: u64 = 0xe10_3ad8_2dad_8028; - -/// A very simple Bloom filter optimized for small values. -#[derive(Clone, Copy)] -pub(crate) struct Bloom { - bits: u64, - data: PhantomData, - entry_count: usize, -} - -#[allow(dead_code)] -impl Bloom { - /// Create a new filter. - pub fn new() -> Self { - Self::default() - } - - /// Returns the number of items that have been added to the filter. - /// - /// Does not count unique entries; this is just the number of times - /// `add()` was called since the filter was created or last `clear()`ed. - // it feels wrong to call this 'len'? - #[cfg(test)] - pub fn entry_count(&self) -> usize { - self.entry_count - } - - /// Return the raw bits of this filter. - #[allow(dead_code)] - pub fn to_raw(&self) -> u64 { - self.bits - } - - /// Remove all entries from the filter. - pub fn clear(&mut self) { - self.bits = 0; - self.entry_count = 0; - } - - /// Add an item to the filter. - pub fn add(&mut self, item: &T) { - let mask = self.make_bit_mask(item); - self.bits |= mask; - self.entry_count += 1; - } - - /// Returns `true` if the item may have been added to the filter. - /// - /// This can return false positives, but never false negatives. - /// Thus `true` means that the item may have been added - or not, - /// while `false` means that the item has definitely not been added. - pub fn may_contain(&self, item: &T) -> bool { - let mask = self.make_bit_mask(item); - self.bits & mask == mask - } - - /// Create a new `Bloom` with the items from both filters. - pub fn union(&self, other: Bloom) -> Bloom { - Bloom { - bits: self.bits | other.bits, - data: PhantomData, - entry_count: self.entry_count + other.entry_count, - } - } - - #[inline] - fn make_bit_mask(&self, item: &T) -> u64 { - //NOTE: we use two hash functions, which performs better than a single hash - // with smaller numbers of items, but poorer with more items. Threshold - // (given 64 bits) is ~30 items. - // The reasoning is that with large numbers of items we're already in bad shape; - // optimize for fewer false positives as we get closer to the leaves. - // This can be tweaked after profiling. - let hash1 = self.make_hash(item, OFFSET_ONE); - let hash2 = self.make_hash(item, OFFSET_TWO); - (1 << (hash1 % NUM_BITS)) | (1 << (hash2 % NUM_BITS)) - } - - #[inline] - fn make_hash(&self, item: &T, seed: u64) -> u64 { - let mut hasher = FnvHasher::with_key(seed); - item.hash(&mut hasher); - hasher.finish() - } -} - -impl std::fmt::Debug for Bloom { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - write!(f, "Bloom: {:064b}: ({})", self.bits, self.entry_count) - } -} - -impl Default for Bloom { - fn default() -> Self { - Bloom { - bits: 0, - data: PhantomData, - entry_count: 0, - } - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn very_good_test() { - let mut bloom = Bloom::default(); - for i in 0..100 { - bloom.add(&i); - assert!(bloom.may_contain(&i)); - } - bloom.clear(); - for i in 0..100 { - assert!(!bloom.may_contain(&i)); - } - } - - #[test] - fn union() { - let mut bloom1 = Bloom::default(); - bloom1.add(&0); - bloom1.add(&1); - assert!(!bloom1.may_contain(&2)); - assert!(!bloom1.may_contain(&3)); - let mut bloom2 = Bloom::default(); - bloom2.add(&2); - bloom2.add(&3); - assert!(!bloom2.may_contain(&0)); - assert!(!bloom2.may_contain(&1)); - - let bloom3 = bloom1.union(bloom2); - assert!(bloom3.may_contain(&0)); - assert!(bloom3.may_contain(&1)); - assert!(bloom3.may_contain(&2)); - assert!(bloom3.may_contain(&3)); - } -} diff --git a/masonry/src/contexts.rs b/masonry/src/contexts.rs index 01660bfb..08580b3d 100644 --- a/masonry/src/contexts.rs +++ b/masonry/src/contexts.rs @@ -674,18 +674,6 @@ impl EventCtx<'_> { } impl LifeCycleCtx<'_> { - /// Registers a child widget. - /// - /// This should only be called in response to a `LifeCycle::WidgetAdded` event. - /// - /// In general, you should not need to call this method; it is handled by - /// the `WidgetPod`. - // TODO - See https://github.com/linebender/xilem/issues/372 - pub(crate) fn register_child(&mut self, child_id: WidgetId) { - trace!("register_child id={:?}", child_id); - self.widget_state.children.add(&child_id); - } - /// Register this widget to be eligile to accept focus automatically. /// /// This should only be called in response to a [`LifeCycle::BuildFocusChain`] event. diff --git a/masonry/src/lib.rs b/masonry/src/lib.rs index f06abf00..d10bf530 100644 --- a/masonry/src/lib.rs +++ b/masonry/src/lib.rs @@ -105,7 +105,6 @@ pub use vello::kurbo; mod util; mod action; -mod bloom; mod box_constraints; mod contexts; mod event; diff --git a/masonry/src/widget/tests/lifecycle_basic.rs b/masonry/src/widget/tests/lifecycle_basic.rs index 281b490b..cb4e0a87 100644 --- a/masonry/src/widget/tests/lifecycle_basic.rs +++ b/masonry/src/widget/tests/lifecycle_basic.rs @@ -57,34 +57,6 @@ fn adding_child() { assert_debug_snapshot!(record_new_child); } -#[test] -fn child_tracking() { - let [id_1, id_2, id_3, id_4] = widget_ids(); - - let widget = SizedBox::new_with_id( - SizedBox::new_with_id( - Flex::row() - .with_child_id(SizedBox::empty(), id_1) - .with_child_id(SizedBox::empty(), id_2), - id_3, - ), - id_4, - ); - - let harness = TestHarness::create(widget); - - let root_state = harness.get_widget(id_4).state(); - assert_eq!(root_state.children.entry_count(), 3); - assert!(root_state.children.may_contain(&id_1)); - assert!(root_state.children.may_contain(&id_2)); - assert!(root_state.children.may_contain(&id_3)); - - let child_state = harness.get_widget(id_3).state(); - assert!(child_state.children.may_contain(&id_1)); - assert!(child_state.children.may_contain(&id_2)); - assert_eq!(child_state.children.entry_count(), 2); -} - /// Test that all children are registered correctly after a child is replaced. #[test] #[cfg(FALSE)] diff --git a/masonry/src/widget/widget_pod.rs b/masonry/src/widget/widget_pod.rs index afd2c9bf..dc5ded91 100644 --- a/masonry/src/widget/widget_pod.rs +++ b/masonry/src/widget/widget_pod.rs @@ -307,14 +307,7 @@ impl WidgetPod { let call_widget = match event { LifeCycle::Internal(internal) => match internal { - InternalLifeCycle::RouteWidgetAdded => { - // TODO - explain - if state.children_changed { - // TODO - Separate "widget removed" case. - state.children.clear(); - } - state.children_changed - } + InternalLifeCycle::RouteWidgetAdded => state.children_changed, InternalLifeCycle::RouteDisabledChanged => { state.update_focus_chain = true; @@ -357,13 +350,9 @@ impl WidgetPod { state.has_focus = false; } - // Recurse when the target widgets could be our descendants. - // The bloom filter we're checking can return false positives. - match (old, new) { - (Some(old), _) if state.children.may_contain(old) => true, - (_, Some(new)) if state.children.may_contain(new) => true, - _ => false, - } + // TODO - This returns a lot of false positives. + // We'll remove this code soon anyway. + matches!((old, new), (Some(_), _) | (_, Some(_))) } }, LifeCycle::WidgetAdded => { @@ -437,9 +426,6 @@ impl WidgetPod { // we need to (re)register children in case of one of the following events LifeCycle::Internal(InternalLifeCycle::RouteWidgetAdded) => { state.children_changed = false; - parent_ctx.widget_state.children = - parent_ctx.widget_state.children.union(state.children); - parent_ctx.register_child(self.id()); } LifeCycle::DisabledChanged(_) | LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged) => { diff --git a/masonry/src/widget/widget_ref.rs b/masonry/src/widget/widget_ref.rs index e56e8331..2f85ea5b 100644 --- a/masonry/src/widget/widget_ref.rs +++ b/masonry/src/widget/widget_ref.rs @@ -230,16 +230,6 @@ impl<'w> WidgetRef<'w, dyn Widget> { for child in self.children() { child.debug_validate(after_layout); - - if !self.state().children.may_contain(&child.state().id) { - debug_panic!( - "Widget '{}' #{} is invalid: child widget '{}' #{} not registered in children filter", - self.deref().short_type_name(), - self.state().id.to_raw(), - child.deref().short_type_name(), - child.state().id.to_raw(), - ); - } } } } diff --git a/masonry/src/widget/widget_state.rs b/masonry/src/widget/widget_state.rs index 2724c4d7..5888846d 100644 --- a/masonry/src/widget/widget_state.rs +++ b/masonry/src/widget/widget_state.rs @@ -6,7 +6,6 @@ use std::sync::atomic::{AtomicBool, Ordering}; use vello::kurbo::{Insets, Point, Rect, Size, Vec2}; -use crate::bloom::Bloom; use crate::text_helpers::TextFieldRegistration; use crate::{CursorIcon, WidgetId}; @@ -115,7 +114,6 @@ pub struct WidgetState { pub(crate) focus_chain: Vec, - pub(crate) children: Bloom, pub(crate) children_changed: bool, // TODO - Remove and handle in WidgetRoot instead @@ -185,7 +183,6 @@ impl WidgetState { has_focus: false, request_anim: true, focus_chain: Vec::new(), - children: Bloom::new(), children_changed: true, cursor: None, is_explicitly_disabled_new: false,