-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: optimize Table::clone_structure
by avoiding clones
#1090
Conversation
2df0cb5
to
bcbbb55
Compare
- Arcing `TableSchema`, and this has benefits elsewhere too. - Arc<[_]>ing the visitor program instructions. The data behind the Arcs very rarely change, which is the perfect case for an Arc.
bcbbb55
to
6d87288
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me.
@@ -622,8 +623,7 @@ impl spacetimedb_commitlog::payload::txdata::Visitor for ReplayVisitor<'_> { | |||
) -> std::result::Result<Self::Row, Self::Error> { | |||
let schema = self | |||
.committed_state | |||
.schema_for_table(&ExecutionContext::default(), table_id)? | |||
.into_owned(); | |||
.schema_for_table(&ExecutionContext::default(), table_id)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a noticeable improvement 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a call with @Centril. Makes sense to me.
Description of Changes
Fixes #1082
Fixes #1084
Make Table::clone_structure cheaper by:
TableSchema
, and this has benefits elsewhere too.The data behind the Arcs very rarely change,
which is the perfect case for an Arc.
API and ABI breaking changes
None
Expected complexity level and risk
2
Testing
No semantic changes nor anything that is very interesting, so existing tests should suffice.