Skip to content
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

DataRef: borrow for great perf #434

Merged
merged 3 commits into from
Oct 19, 2023
Merged

DataRef: borrow for great perf #434

merged 3 commits into from
Oct 19, 2023

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Oct 16, 2023

Description of Changes

Get rid of the old DataRef so that it instead borrows the DataKey and ProductValue.
Then this is used throughout so that iter, iter_by_col_eq, table_name_from_id, and friends, do not .clone() and instead just returns or works off of borrowed data.

API and ABI

  • This is a breaking change to the module ABI
  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

Expected complexity level and risk

2

@Centril Centril force-pushed the a-little-less-clone branch 3 times, most recently from 417a012 to 99ceab0 Compare October 17, 2023 09:22
Copy link
Collaborator

@joshua-spacetime joshua-spacetime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small suggestions, but otherwise this is great. Would you be able to upload a before and after flamegraph to show the improvements?

@gefjon
Copy link
Contributor

gefjon commented Oct 17, 2023

Would you be able to upload a before and after flamegraph to show the improvements?

I have these flamegraphs, but I don't have a good way to upload them - the perf flamegraph script generates HTML, which GitHub doesn't allow as an attachment.

@joshua-spacetime
Copy link
Collaborator

Uploading before and after screenshots of some flamegraphs that @gefjon generated.
Screenshot 2023-10-17 at 12 02 10 PM
Screenshot 2023-10-17 at 12 02 42 PM
A roughly 6% reduction in iteration time (next)

…_table` (#436)

* optimize delete, drop, rename

* address review comments

* even less cloning

* more refactoring
@Centril Centril enabled auto-merge (squash) October 19, 2023 18:07
@Centril Centril merged commit c8f9a29 into master Oct 19, 2023
5 checks passed

Ok(count)
let count = stdb.delete(tx, table_id, rows_to_delete);
NonZeroU32::new(count).ok_or(NodesError::ColumnValueNotFound)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this introduced the new log lines we are discussing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so we don't want this to be an error? Easy enough to return an u32 instead and remove line 164.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #539.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants