struct LazyMapOfIndexSet: #708

MilkBlock · 2025-10-14T01:53:35Z

opt DashMap<Value,IndexSet> by allowing lazily insert when read

opt DashMap<Value,IndexSet<Value>> by allowing lazily insert when read

codspeed-hq · 2025-10-14T02:11:09Z

CodSpeed Performance Report

Merging #708 will improve performances by ×2.9

_{Comparing MilkBlock:im_main (8d6167d) with main (5678c6c)}

Summary

⚡ 2 improvements
✅ 18 untouched
⏩ 190 skipped¹

Benchmarks breakdown

	Mode	Benchmark	`BASE`	`HEAD`	Change
⚡	WallTime	`tests[repro-665-set-union]`	1,031.8 ms	358.1 ms	×2.9
⚡	Simulation	`tests[repro-665-set-union]`	781.7 ms	541 ms	+44.49%

190 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

MilkBlock · 2025-10-14T02:12:07Z

@saulshanabrook

saulshanabrook · 2025-10-14T02:19:16Z

Thanks for this change! Looks like it does improve performance significantly. @ezrosent said he can take a look, he is more familiar with this code.

MilkBlock · 2025-10-14T03:35:47Z

I don't quite understand what happended to [test]map. I simply refuse lazy insertion when the size of keys are too small.

https://codspeed.io/egraphs-good/egglog/runs/compare/68edafb692213580f5f86323..68edc1970851b28d201a68e7

MilkBlock · 2025-10-14T05:20:26Z

No idea why string_quotes get slower. It's not even relevent to containers.

saulshanabrook · 2025-10-14T05:58:59Z

I am going to go back to hiding all the fast running benchmarks. They generally have had too much uncertainty to be that helpful to us in the past.

yihozhang · 2025-10-23T07:08:38Z

core-relations/src/containers/mod.rs

+    /// Flushes all pending lazy insertions to the underlying map.
+    fn flush_pending_operations_for_key(&self, key: &Value) {
+        let mut pending_ops = self.pending_operations.lock().unwrap();
+        if !pending_ops.is_empty() {


this if statement is redundant?

Currently, pending_operations never shrinks, even when the actual val_index is small.

We can maybe do a pending_ops.retain(|(keys, op)| !keys.is_empty()) when we find there are too many empty sets during the enumeration

Plausible, added

yihozhang · 2025-10-23T07:27:06Z

core-relations/src/containers/mod.rs

-                                    index.swap_remove(&old_val);
-                                    index.insert(result);
-                                }
+                                self.val_index


swap_remove is always used together with an insert, so maybe we can have an update_for_all_keys that takes both the old value and the new value. This way you don't need to materialize the container twice.

As another optimization, currently we always materialize the container (container.iter().collect()), but when the container only has a fewer entries than (LAZY_BOUND), the materialized container is immediately discarded. Can we make insert/remove_for_all_keys take an iterator instead of an IndexSet?

2 ways to implement this.

pub trait ContainerValue: Hash + Eq + Clone + Send + Sync + 'static { /// Rebuild an additional container in place according the the given [`Rebuilder`]. /// /// If this method returns `false` then the container must not have been modified (i.e. it must /// hash to the same value, and compare equal to a copy of itself before the call). fn rebuild_contents(&mut self, rebuilder: &dyn Rebuilder) -> bool; /// Iterate over the contents of the container. /// /// Note that containers can be more structured than just a sequence of values. This iterator /// is used to populate an index that in turn is used to speed up rebuilds. If a value in the /// container is eligible for a rebuild and it is not mentioned by this iterator, the outer /// [`Containers`] registry may skip rebuilding this container. fn iter(&self) -> impl Iterator<Item = Value> + '_; }

trait revision: add fn len()to this trait

make iterator sized_iterator

Maybe we could consider it in next PR this may require many changes.

Can we just change the interface of iter to return an ExtractSizeIterator?

yihozhang · 2025-10-23T07:31:13Z

core-relations/src/containers/mod.rs

+    // keys and value to insert
+    // if user want to insert same value for all keys in IndexSet<Value>, LazyMap will put them
+    // in pending_insert and do the insertion for single key and remove this key in pending_insert when user want to read LazyMap
+    pending_operations: Arc<Mutex<Vec<(IndexSet<Value>, InsertOrRemove)>>>,


nit: it does not need to be an IndexSet, and a HashSet should be good enough?

yihozhang · 2025-10-23T07:37:54Z

core-relations/src/containers/mod.rs

+    val_index: LazyMapOfIndexSet,
+}
+#[derive(Clone)]
+struct LazyMapOfIndexSet {


nit: consider renaming to just LazyValIndex or LazyContainerIndex?

yihozhang · 2025-10-23T07:39:23Z

core-relations/src/containers/mod.rs

+
+const LAZY_BOUND: usize = 30;
+use dashmap::mapref::one::{Ref, RefMut};
+#[allow(dead_code)]


I think we should be able to remove the dead code? Unused functions can be re-implemented later easily if we want to.

yihozhang · 2025-10-23T07:52:29Z

Also, are you sure this PR makes #665 faster?

➜  egglog git:(main) git checkout im_main
branch 'im_main' set up to track 'milkblock/im_main'.
Switched to a new branch 'im_main'
➜  egglog git:(im_main) cargo build --release
   Compiling egglog v1.0.0 (/home/yz489/egglog)
   Compiling egglog-core-relations v1.0.0 (/home/yz489/egglog/core-relations)
   Compiling egglog-bridge v1.0.0 (/home/yz489/egglog/egglog-bridge)
    Finished `release` profile [optimized] target(s) in 37.08s
➜  egglog git:(im_main) time target/release/egglog tests/repro-665-set-union.egg
target/release/egglog tests/repro-665-set-union.egg  0.53s user 0.02s system 99% cpu 0.554 total
➜  egglog git:(im_main) gco main
Switched to branch 'main'
Your branch is up to date with 'origin/main'.
➜  egglog git:(main) cargo build --release
   Compiling egglog v1.0.0 (/home/yz489/egglog)
   Compiling egglog-numeric-id v1.0.0 (/home/yz489/egglog/numeric-id)
   Compiling egraph-serialize v0.3.0
   Compiling egglog-union-find v1.0.0 (/home/yz489/egglog/union-find)
   Compiling egglog-core-relations v1.0.0 (/home/yz489/egglog/core-relations)
   Compiling egglog-bridge v1.0.0 (/home/yz489/egglog/egglog-bridge)
    Finished `release` profile [optimized] target(s) in 38.57s
➜  egglog git:(main) time target/release/egglog tests/repro-665-set-union.egg
target/release/egglog tests/repro-665-set-union.egg  0.37s user 0.02s system 99% cpu 0.390 total

If you look at the flamegraph on codspeed, most of the time is spent in dropping the E-graph... But in CLI mode, the E-graph is mem::forget, so that cost is avoided.

MilkBlock · 2025-10-23T09:26:06Z

➜  egglog git:(im_main) ✗ gco im_main                                                 
Already on 'im_main'
Your branch is up to date with 'myfork/im_main'.
➜  egglog git:(im_main) ✗ cargo build --release                                       
    Finished `release` profile [optimized] target(s) in 0.14s
➜  egglog git:(im_main) ✗ time ./target/release/egglog ./tests/repro-665-set-union.egg
./target/release/egglog ./tests/repro-665-set-union.egg  0.07s user 0.01s system 85% cpu 0.090 total
➜  egglog git:(im_main) ✗ gco main                                                    
Switched to branch 'main'
Your branch is up to date with 'myfork/main'.
➜  egglog git:(main) ✗ cargo build --release                                       
   Compiling egglog-core-relations v1.0.0 (/Users/mineralsteins/Repos/stable/egglog/core-relations)
   Compiling egglog v1.0.0 (/Users/mineralsteins/Repos/stable/egglog)
   Compiling egglog-bridge v1.0.0 (/Users/mineralsteins/Repos/stable/egglog/egglog-bridge)
    Finished `release` profile [optimized] target(s) in 22.64s
➜  egglog git:(main) ✗ time ./target/release/egglog ./tests/repro-665-set-union.egg
./target/release/egglog ./tests/repro-665-set-union.egg  0.12s user 0.01s system 92% cpu 0.149 total

emmm seems different on my machine.
I would have a check.

main branch : d3c80a4 (HEAD -> main, myfork/main) container_to_value in top level EGraph and comment revision
im_main branch : d10d939 (HEAD -> im_main, myfork/im_main) nit

Also, on this graph you can see the performance is improved in run_schedule but not egraph struct memory drop.

MilkBlock · 2025-10-23T10:03:56Z

The graph you saw might be a function name display bug on codspeed. You can see the drop_in_place function takes 80% of time, which is hard to believe.

I think the performance also depends on the number of cores and this is my hardware.

  Hardware Overview:

      Model Name: MacBook Pro
      Model Identifier: Mac15,9
      Model Number: MUW63CH/A
      Chip: Apple M3 Max
      Total Number of Cores: 16 (12 performance and 4 efficiency)
      Memory: 48 GB

I don't quite understand performance regression on your machine, maybe you could perf it?

saulshanabrook · 2025-10-23T11:48:44Z

But in CLI mode, the E-graph is mem::forget, so that cost is avoided.

I opened #718 to track forgetting in the benchmark as well to get more similar performance

saulshanabrook · 2025-10-23T12:00:43Z

The graph you saw might be a function name display bug on codspeed. You can see the drop_in_place function takes 80% of time, which is hard to believe.

I opened a support request for this in the codspeed discord.

yihozhang · 2025-10-23T22:23:01Z

I can confirm similar speedups after rebasing from main. I think it's because of the #709

yihozhang · 2025-10-23T16:07:53Z

core-relations/src/containers/mod.rs

+
+    /// Lazily removes a value for all keys in the given index set.
+    pub fn remove_for_all_keys(&self, keys: HashSet<Value>, value: Value) {
+        if keys.len() < LAZY_BOUND {


Need to flush all the updates before eagerly removing?

update: I think you also need to do flush_pending_operations_for_key for eager insertion, not just eager removal

yihozhang · 2025-10-23T16:08:28Z

core-relations/src/containers/mod.rs

+    // keys and value to insert
+    // if user want to insert same value for all keys in IndexSet<Value>, LazyMap will put them
+    // in pending_insert and do the insertion for single key and remove this key in pending_insert when user want to read LazyMap
+    pending_operations: Arc<Mutex<Vec<(HashSet<Value>, InsertOrRemove)>>>,


This will be very slow when many threads are contending to insert to the index

Eli suggested crossbeam_queue for concurrent access.

saulshanabrook · 2025-10-25T04:26:57Z

From codspeed on discord fyi:

Hey, indeed there is an ongoing issue on our side with walltime flamegraphs. We are currently fixing. I will let you know when it is fixed!

struct LazyMapOfIndexSet:

dc66571

opt DashMap<Value,IndexSet<Value>> by allowing lazily insert when read

MilkBlock requested a review from a team as a code owner October 14, 2025 01:53

MilkBlock requested review from oflatt and removed request for a team October 14, 2025 01:53

support lazy remove

6d51fd5

MineralSteins added 2 commits October 14, 2025 12:48

with LAZY_BOUND

38a041d

nit

d10d939

yihozhang requested changes Oct 23, 2025

View reviewed changes

rename & clear empty set & swith impl to hashset

8d6167d

saulshanabrook mentioned this pull request Oct 23, 2025

Remove time for dropping egraph in benchmarks #718

Open

yihozhang reviewed Oct 23, 2025

View reviewed changes

MilkBlock marked this pull request as draft November 7, 2025 01:32

struct LazyMapOfIndexSet: #708

Are you sure you want to change the base?

struct LazyMapOfIndexSet: #708

Uh oh!

Conversation

MilkBlock commented Oct 14, 2025

Uh oh!

codspeed-hq bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #708 will improve performances by ×2.9

Summary

Benchmarks breakdown

Footnotes

Uh oh!

MilkBlock commented Oct 14, 2025

Uh oh!

saulshanabrook commented Oct 14, 2025

Uh oh!

MilkBlock commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MilkBlock commented Oct 14, 2025

Uh oh!

saulshanabrook commented Oct 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MilkBlock Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihozhang commented Oct 23, 2025

Uh oh!

MilkBlock commented Oct 23, 2025 • edited by saulshanabrook Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MilkBlock commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saulshanabrook commented Oct 23, 2025

Uh oh!

saulshanabrook commented Oct 23, 2025

Uh oh!

yihozhang commented Oct 23, 2025

Uh oh!

yihozhang Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saulshanabrook commented Oct 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codspeed-hq bot commented Oct 14, 2025 •

edited

Loading

MilkBlock commented Oct 14, 2025 •

edited

Loading

MilkBlock Oct 23, 2025 •

edited

Loading

MilkBlock commented Oct 23, 2025 •

edited by saulshanabrook

Loading

MilkBlock commented Oct 23, 2025 •

edited

Loading

yihozhang Oct 23, 2025 •

edited

Loading