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

remove transactions copy #447

Merged

Conversation

iceseer
Copy link
Contributor

@iceseer iceseer commented Apr 14, 2020

Signed-off-by: iceseer iceseer@gmail.com

Description of the Change

Performance fix.

Benefits

Possible Drawbacks

Usage Examples or Tests [optional]

Alternate Designs [optional]

@iceseer iceseer requested a review from lebdron April 15, 2020 12:34
@iceseer iceseer force-pushed the fix/remove_transactions_copy branch from 120d9cd to d73377c Compare April 15, 2020 14:06
@iceseer iceseer requested a review from MBoldyrev April 15, 2020 15:25
@lebdron lebdron added the 1.x label Apr 16, 2020
Copy link
Contributor

@MBoldyrev MBoldyrev left a comment

Choose a reason for hiding this comment

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

Generally, shared objects should be const so that they would not change when not expected to. I would like to keep shared pointer to const block in the interfaces. The change you make does not demand the interface to use non-const shared block.

IMHO, what is wrong here is the block creator (fetch method) creating a const object. To be consistent with my first point, this producer should rather create a non-const unique pointer, and if the component that gets this block needs to share it, it is responsible for making it a shared const. If it does not need to share, then it has a non-const block that it can move transactions from without any risk of messing someone other's view.

Comment on lines 36 to 37
std::shared_ptr<shared_model::interface::Block>>
fetch(shared_model::interface::types::HeightType height) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::shared_ptr<shared_model::interface::Block>>
fetch(shared_model::interface::types::HeightType height) = 0;
std::unique_ptr<shared_model::interface::Block>>
fetch(shared_model::interface::types::HeightType height) const = 0;

@@ -53,7 +53,7 @@ namespace iroha {
/**
* Iterates through all the stored blocks
*/
virtual void forEach(FunctionType function) const = 0;
virtual void forEach(FunctionType function) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
virtual void forEach(FunctionType function) = 0;
virtual void forEach(FunctionType function) const = 0;

shared_model/backend/protobuf/block.hpp Outdated Show resolved Hide resolved
test/module/irohad/ametsuchi/mock_block_storage.hpp Outdated Show resolved Hide resolved
test/module/irohad/ametsuchi/mock_block_storage.hpp Outdated Show resolved Hide resolved
@iceseer iceseer force-pushed the fix/remove_transactions_copy branch from bb80a3e to 790e75a Compare April 28, 2020 09:23
@iceseer iceseer force-pushed the fix/remove_transactions_copy branch 2 times, most recently from aeeb3f6 to 0f2863c Compare May 21, 2020 16:19
@iceseer iceseer added this to the 1.2 milestone May 29, 2020
@lebdron lebdron force-pushed the fix/remove_transactions_copy branch from a8ccf81 to 044a412 Compare June 5, 2020 10:00
iceseer and others added 2 commits June 5, 2020 13:37
Signed-off-by: iceseer <iceseer@gmail.com>
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron lebdron force-pushed the fix/remove_transactions_copy branch from 044a412 to d3c6323 Compare June 5, 2020 10:38
@lebdron lebdron merged commit 97a6009 into hyperledger-iroha:master Jun 5, 2020
@nxsaken nxsaken added the iroha1 The legacy version of Iroha. label Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x iroha1 The legacy version of Iroha.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants