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

Prevent unnecessary shared_ptr copies by accepting a value in SHAMapInnerNode::setChild #4266

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

greg7mdp
Copy link
Contributor

@greg7mdp greg7mdp commented Aug 4, 2022

High Level Overview of Change

Because SHAMapInnerNode::setChild takes a shared_ptr const &, every node insertion requires a shared_ptr copy, which causes an increment/decrement of the refcount in the object. This PR makes SHAMapInnerNode::setChild take a value which is moved into place.

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ x] Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

This does not cause any functional change.

Copy link
Contributor

@nbougalis nbougalis left a comment

Choose a reason for hiding this comment

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

Preliminary veto. See below comments.

src/ripple/shamap/impl/SHAMap.cpp Show resolved Hide resolved
src/ripple/shamap/impl/SHAMap.cpp Outdated Show resolved Hide resolved
@nbougalis nbougalis added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 30, 2022
@intelliot intelliot removed the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 28, 2022
@intelliot
Copy link
Collaborator

(note from discussion: we'd like 1 more review of this)

Copy link
Collaborator

@mtrippled mtrippled left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@intelliot intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 29, 2022
@intelliot intelliot merged commit c1e7fe2 into XRPLF:develop Dec 9, 2022
dangell7 pushed a commit to Transia-RnD/rippled that referenced this pull request Mar 5, 2023
…apInnerNode::setChild` (XRPLF#4266)

* Do a move instead of a copy in `SHAMapInnerNode::setChild`

* Create the value directly in the call
tequdev pushed a commit to tequdev/rippled that referenced this pull request Nov 17, 2023
…apInnerNode::setChild` (XRPLF#4266)

* Do a move instead of a copy in `SHAMapInnerNode::setChild`

* Create the value directly in the call
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants