Implement wal logging and replay for EdgeView, NodeView, delete_edge and graph properties#2493
Implement wal logging and replay for EdgeView, NodeView, delete_edge and graph properties#2493
EdgeView, NodeView, delete_edge and graph properties#2493Conversation
EdgeViewEdgeView write operations
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: f957390 | Previous: 9567900 | Ratio |
|---|---|---|---|
lotr_graph/num_edges |
4 ns/iter (± 0) |
0 ns/iter (± 0) |
+∞ |
lotr_graph/num_nodes |
432 ns/iter (± 1) |
2 ns/iter (± 0) |
216 |
lotr_graph/graph_latest |
3 ns/iter (± 0) |
0 ns/iter (± 0) |
+∞ |
lotr_graph_materialise/materialize |
9270087 ns/iter (± 89816) |
1522661 ns/iter (± 3102) |
6.09 |
lotr_graph_window_100/num_edges |
29 ns/iter (± 0) |
8 ns/iter (± 0) |
3.63 |
lotr_graph_window_100/num_nodes |
486 ns/iter (± 1) |
5 ns/iter (± 0) |
97.20 |
lotr_graph_window_100_materialise/materialize |
9545910 ns/iter (± 64509) |
1647661 ns/iter (± 5147) |
5.79 |
lotr_graph_window_10/has_node_existing |
133 ns/iter (± 9) |
61 ns/iter (± 12) |
2.18 |
lotr_graph_window_10/iterate nodes |
34307 ns/iter (± 110) |
10897 ns/iter (± 52) |
3.15 |
lotr_graph_window_10/iterate edges |
104057 ns/iter (± 228) |
45567 ns/iter (± 1365) |
2.28 |
lotr_graph_window_10_materialise/materialize |
6155433 ns/iter (± 37037) |
1013406 ns/iter (± 5119) |
6.07 |
lotr_graph_subgraph_10pc/has_edge_existing |
520 ns/iter (± 4) |
85 ns/iter (± 1) |
6.12 |
lotr_graph_subgraph_10pc/num_nodes |
354 ns/iter (± 4) |
4 ns/iter (± 0) |
88.50 |
lotr_graph_subgraph_10pc/has_node_existing |
231 ns/iter (± 4) |
30 ns/iter (± 0) |
7.70 |
lotr_graph_subgraph_10pc/iterate nodes |
4679 ns/iter (± 15) |
853 ns/iter (± 1) |
5.49 |
lotr_graph_subgraph_10pc_windowed/has_node_existing |
133 ns/iter (± 7) |
62 ns/iter (± 14) |
2.15 |
lotr_graph_subgraph_10pc_windowed/iterate nodes |
4685 ns/iter (± 37) |
1399 ns/iter (± 3) |
3.35 |
lotr_graph_window_50_layered/num_nodes |
101959 ns/iter (± 1104) |
22515 ns/iter (± 3326) |
4.53 |
lotr_graph_window_50_layered/has_node_existing |
1413 ns/iter (± 206) |
123 ns/iter (± 15) |
11.49 |
lotr_graph_window_50_layered/max_id |
115275 ns/iter (± 1542) |
27871 ns/iter (± 475) |
4.14 |
lotr_graph_window_50_layered/max_degree |
1944492 ns/iter (± 21262) |
298581 ns/iter (± 9998) |
6.51 |
lotr_graph_window_50_layered/iterate nodes |
237823 ns/iter (± 841) |
18512 ns/iter (± 205) |
12.85 |
lotr_graph_window_50_layered/iterate edges |
214859 ns/iter (± 463) |
83040 ns/iter (± 327) |
2.59 |
lotr_graph_window_50_layered/max_neighbour_degree |
3651492 ns/iter (± 16284) |
388874 ns/iter (± 1455) |
9.39 |
lotr_graph_window_50_layered/graph_latest |
183360 ns/iter (± 8579) |
38418 ns/iter (± 718) |
4.77 |
lotr_graph_window_50_layered_materialise/materialize |
21718186 ns/iter (± 378074) |
3378119 ns/iter (± 12216) |
6.43 |
lotr_graph_persistent_window_50_layered/num_edges_temporal |
587197 ns/iter (± 2834) |
207488 ns/iter (± 4439) |
2.83 |
lotr_graph_persistent_window_50_layered/num_nodes |
137021 ns/iter (± 2079) |
31798 ns/iter (± 386) |
4.31 |
lotr_graph_persistent_window_50_layered/has_node_existing |
1741 ns/iter (± 432) |
180 ns/iter (± 82) |
9.67 |
lotr_graph_persistent_window_50_layered/max_id |
151675 ns/iter (± 2146) |
39294 ns/iter (± 1497) |
3.86 |
lotr_graph_persistent_window_50_layered/max_degree |
2764795 ns/iter (± 17389) |
457666 ns/iter (± 12739) |
6.04 |
lotr_graph_persistent_window_50_layered/iterate nodes |
323659 ns/iter (± 802) |
36447 ns/iter (± 154) |
8.88 |
lotr_graph_persistent_window_50_layered/iterate edges |
187474 ns/iter (± 434) |
86397 ns/iter (± 339) |
2.17 |
lotr_graph_persistent_window_50_layered/max_neighbour_degree |
4892212 ns/iter (± 10590) |
518296 ns/iter (± 8441) |
9.44 |
lotr_graph_persistent_window_50_layered/graph_latest |
274699 ns/iter (± 16021) |
58093 ns/iter (± 1228) |
4.73 |
lotr_graph_persistent_window_50_layered_materialise/materialize |
54902456 ns/iter (± 1598088) |
5111080 ns/iter (± 28206) |
10.74 |
lotr_graph/proto_decode |
9583564 ns/iter (± 60049) |
1875628 ns/iter (± 40776) |
5.11 |
lotr_graph/proto_encode |
9230511 ns/iter (± 64127) |
1163713 ns/iter (± 114520) |
7.93 |
This comment was automatically generated by workflow using github-action-benchmark.
EdgeView write operationsEdgeView and NodeView operations
| match prop_mapper.get_dtype(*prop_id) { | ||
| None => { | ||
| prop_mapper.set_id_and_dtype( | ||
| prop_name.as_str(), | ||
| *prop_id, | ||
| prop_value.dtype(), | ||
| ); | ||
| } | ||
| Some(old_dtype) => { | ||
| let dtype = prop_value.dtype(); | ||
| let mut unified = false; | ||
| let new_dtype = unify_types(&old_dtype, &dtype, &mut unified)?; | ||
| if unified { | ||
| prop_mapper.set_dtype(*prop_id, new_dtype); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
set_or_unify_id_and_dtype is not exactly the same as this block, since it always sets the id whereas this doesn't, but minor differences aside lmk if it's ok to use this method here.
There was a problem hiding this comment.
we can always optimise that later, so using that method is fine.
fabianmurariu
left a comment
There was a problem hiding this comment.
Looks good, just a few changes needed
db4-graph/src/replay.rs
Outdated
| )?; | ||
| } | ||
|
|
||
| let mut edge_writer = self.edges.get_mut(edge_segment_id).unwrap().writer(); |
db4-graph/src/replay.rs
Outdated
| )?; | ||
| } | ||
|
|
||
| let mut node_writer = self.nodes.get_mut(segment_id).unwrap().writer(); |
db4-graph/src/replay.rs
Outdated
|
|
||
| let mut node_writer = self.nodes.get_mut(segment_id).unwrap().writer(); | ||
|
|
||
| let props_vec: Vec<_> = props.iter().map(|(_, id, p)| (*id, p.clone())).collect(); |
There was a problem hiding this comment.
I don't think collect is needed
db4-graph/src/replay.rs
Outdated
| )) | ||
| })?; | ||
|
|
||
| let props_vec: Vec<_> = props.iter().map(|(_, id, p)| (*id, p.clone())).collect(); |
There was a problem hiding this comment.
probably needless collect
b7f08a8 to
f69e1bf
Compare
EdgeView and NodeView operationsEdgeView, NodeView, delete_edge and graph properties
ljeub-pometry
left a comment
There was a problem hiding this comment.
Some minor things to clean up
db4-graph/src/replay.rs
Outdated
| // Insert prop ids into edge meta. | ||
| for (prop_name, prop_id, prop_value) in &props { | ||
| let prop_mapper = edge_meta.temporal_prop_mapper(); | ||
| let mut write_locked_mapper = prop_mapper.write_locked(); |
There was a problem hiding this comment.
this should be outside the loop! you keep locking and unlocking the mapper for no reason
There was a problem hiding this comment.
Actually, maybe it would make more sense to have this as part of the initialisation of the write-locked graph itself?
| match prop_mapper.get_dtype(*prop_id) { | ||
| None => { | ||
| prop_mapper.set_id_and_dtype( | ||
| prop_name.as_str(), | ||
| *prop_id, | ||
| prop_value.dtype(), | ||
| ); | ||
| } | ||
| Some(old_dtype) => { | ||
| let dtype = prop_value.dtype(); | ||
| let mut unified = false; | ||
| let new_dtype = unify_types(&old_dtype, &dtype, &mut unified)?; | ||
| if unified { | ||
| prop_mapper.set_dtype(*prop_id, new_dtype); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
we can always optimise that later, so using that method is fine.
db4-graph/src/replay.rs
Outdated
|
|
||
| for (prop_name, prop_id, prop_value) in &props { | ||
| let prop_mapper = edge_meta.metadata_mapper(); | ||
| let mut write_locked_mapper = prop_mapper.write_locked(); |
There was a problem hiding this comment.
As before, we should not have this inside the loop. Probably lock these as part of the write-locked graph initialisation.
| lsn: LSN, | ||
| _transaction_id: TransactionID, | ||
| t: EventTime, | ||
| _src_name: Option<GID>, |
There was a problem hiding this comment.
It is possible to create new nodes on a delete_edge call, you need to handle these!
db4-graph/src/replay.rs
Outdated
| } | ||
| } | ||
| } | ||
| let mut write_locked_mapper = prop_mapper.write_locked(); |
There was a problem hiding this comment.
As before, don't call this in the loop.
db4-graph/src/replay.rs
Outdated
| })?; | ||
|
|
||
| let mut node_writer = node_writer.writer(); | ||
| let props = props.iter().map(|(_, id, p)| (*id, p.clone())); |
There was a problem hiding this comment.
use into_iter to avoid the clone
db4-graph/src/replay.rs
Outdated
| )) | ||
| })?; | ||
|
|
||
| let props = props.iter().map(|(_, id, p)| (*id, p.clone())); |
There was a problem hiding this comment.
use into_iter to avoid the clone
db4-graph/src/replay.rs
Outdated
| })?; | ||
|
|
||
| let mut node_writer = node_writer.writer(); | ||
| let props = props.iter().map(|(_, id, p)| (*id, p.clone())); |
There was a problem hiding this comment.
use into_iter to avoid the clone
db4-storage/src/pages/node_store.rs
Outdated
| // The current segment is full, drop its lock and try to find | ||
| // another free segment. |
There was a problem hiding this comment.
We are not trying to find a free segment here, we are going to replace the segment at this slot with a new one (unless another thread managed to replace it before we get to it)
| } | ||
|
|
||
| fn immut_lsn(&self) -> LSN { | ||
| self.head.read().lsn() |
There was a problem hiding this comment.
That doesn't seem like the immut_lsn since you are reading the mutable part?
Adds wal logging and replay for
add_metadata,update_metadataandadd_updateson anEdgeViewandNodeView.Additionally adds wal support for
delete_edgeand all operations on graph properties.