Cache the 'row_type' on TableSchema so is not re-build in each access #638
Conversation
| table_id: TableId(42), | ||
| table_name: "Person".into(), | ||
| columns: vec
|
benchmark please |
| @@ -271,7 +271,7 @@ impl CommittedState { | |||
| // Insert the columns into `st_columns` | |||
| let st_columns = self.get_or_create_table(ST_COLUMNS_ID, st_columns_schema()); | |||
|
|
|||
| for col in system_tables().into_iter().flat_map(|x| x.columns) { | |||
| for col in system_tables().into_iter().flat_map(|x| x.columns.clone()) { | |||
There was a problem hiding this comment.
Why is this cloning now? You already have an owned type due to into_iter.
There was a problem hiding this comment.
Rust complains:
error[E0507]: cannot move out of dereference of `spacetimedb_sats::db::def::TableSchema`
--> crates/core/src/db/datastore/locking_tx_datastore/mod.rs:274:61
|
274 | for col in system_tables().into_iter().flat_map(|x| x.columns) {
| ^^^^^^^^^ move occurs because value has type `std::vec::Vec<spacetimedb_sats::db::def::ColumnSchema>`, which does not implement the `Copy` trait
I think this is a consequence of the read_only mark.
There was a problem hiding this comment.
Ah, well in that case I think the attribute should be removed.
There was a problem hiding this comment.
The issue is that caching the row_type means any change to columns invalidates that, and I need a way to protect against that scenario.
There was a problem hiding this comment.
You can make the columns field non-pub in that case and provide methods that give self and &self access but not &mut self.
| self.schema_for_table(table_id, database_address)?.get_row_type(), | ||
| self.schema_for_table(table_id, database_address)? | ||
| .get_row_type() | ||
| .clone(), |
There was a problem hiding this comment.
schema_for_table' Cow::Borrowed branch should be unreachable here because the if let Some(row_type) branch wasn't taken. At any rate, you can avoid this clone by checking if we have Cow::Owned.
| )) | ||
| Ok(match self.schema_for_table(table_id, database_address)? { | ||
| Cow::Borrowed(x) => Cow::Borrowed(x.get_row_type()), | ||
| Cow::Owned(x) => Cow::Owned(x.get_row_type().clone()), |
There was a problem hiding this comment.
clone shouldn't be necessary in the Owned branch either.
There was a problem hiding this comment.
This is get_row_type(&self) -> &ProductType
There was a problem hiding this comment.
Yeah but you have ownership of TableSchema, so you could get out an owned ProductType, either through adding an additional method or by making the field pub.
mamcx
force-pushed
the
mamcx/perf-multi-column
branch
from
5c08141 to
c1f1148
Compare
cloutiertyler
dismissed
Centril’s stale review
I'm dismissing this because the comments appear to me to have been addressed.
Description of Changes
This is a possible fix for the performance issue on BitCraft.
Now is forbidden to mutate the
columnsonTableSchemaso we can cache therow_type.Expected complexity level and risk
1