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

[PART 2] Taking operator for Tx pool #1645

Closed
wants to merge 10 commits into from
Closed

Conversation

evgenykuzyakov
Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov commented Nov 5, 2019

@nearmax Does this make sense so far?

@evgenykuzyakov evgenykuzyakov requested a review from nearmax Nov 5, 2019
@codecov
Copy link

@codecov codecov bot commented Nov 5, 2019

Codecov Report

Merging #1645 into improve-tx-pool will increase coverage by 0.07%.
The diff coverage is 97.84%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           improve-tx-pool    #1645      +/-   ##
===================================================
+ Coverage            79.79%   79.87%   +0.07%     
===================================================
  Files                  130      130              
  Lines                24041    24108      +67     
===================================================
+ Hits                 19184    19256      +72     
+ Misses                4857     4852       -5
Impacted Files Coverage Δ
chain/pool/src/lib.rs 98.19% <97.84%> (+0.71%) ⬆️
runtime/runtime/src/metrics.rs 36.36% <0%> (-5.31%) ⬇️
chain/jsonrpc/client/src/message.rs 82.04% <0%> (-1.09%) ⬇️
chain/client/tests/query_client.rs 92.85% <0%> (-0.48%) ⬇️
chain/client/tests/cross_shard_tx.rs 19.2% <0%> (-0.25%) ⬇️
runtime/runtime/src/state_viewer.rs 89.55% <0%> (-0.23%) ⬇️
runtime/runtime/src/balance_checker.rs 94.27% <0%> (-0.09%) ⬇️
chain/network/src/routing.rs 93.12% <0%> (-0.06%) ⬇️
chain/jsonrpc/tests/rpc_query.rs 99.23% <0%> (-0.02%) ⬇️
chain/chain/tests/finality.rs 99.5% <0%> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5faa4bb...2df5904. Read the comment docs.

@evgenykuzyakov
Copy link
Collaborator Author

@evgenykuzyakov evgenykuzyakov commented Nov 5, 2019

Needs a fix for invalid transactions, since they going to skip the key and invalidate the order.

@evgenykuzyakov evgenykuzyakov marked this pull request as ready for review Nov 6, 2019
@evgenykuzyakov evgenykuzyakov changed the title Taking operator for Tx pool [PART 2] Taking operator for Tx pool Nov 6, 2019
@nearmax
Copy link
Member

@nearmax nearmax commented Nov 6, 2019

@nearmax Does this make sense so far?

So this PR is rewrites the code in #1591 ?

@evgenykuzyakov
Copy link
Collaborator Author

@evgenykuzyakov evgenykuzyakov commented Nov 6, 2019

@nearmax Yes, it implements an iterator which allows to retry elements for the same key for skipped invalid transactions. It's necessary to keep the proper order.

chain/pool/src/lib.rs Outdated Show resolved Hide resolved
/// Returns a draining structure that pulls transactions from the pool in the proper order.
/// It has an option to take transactions with the same key as the last one or with the new key.
/// When the iterator is dropped, the rest of the transactions remains in the pool.
pub fn draining_iterator(&mut self) -> DrainingIterator {
Copy link
Member

@nearmax nearmax Nov 6, 2019

Choose a reason for hiding this comment

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

Suggested change
pub fn draining_iterator(&mut self) -> DrainingIterator {
fn draining_iterator(&mut self) -> DrainingIterator {

Copy link
Collaborator Author

@evgenykuzyakov evgenykuzyakov Nov 6, 2019

Choose a reason for hiding this comment

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

In the Part 3, I'm only using draining_iterator and not prepare_transactions

chain/pool/src/lib.rs Outdated Show resolved Hide resolved
let mut res = vec![];
let mut iter = self.draining_iterator();
for _ in 0..max_number_of_transactions {
if let Some(tx) = iter.next(false) {
Copy link
Member

@nearmax nearmax Nov 6, 2019

Choose a reason for hiding this comment

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

Since iterator is used in the same place it is created we can implement Iterator for DrainingIterator and then do

self.draining_iterator().take(max_number_of_transactions).collect()

If it wasn't used in the same place we would've needed some lifetimes trickery.

Copy link
Collaborator Author

@evgenykuzyakov evgenykuzyakov Nov 7, 2019

Choose a reason for hiding this comment

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

This goes away in part 3 as well (towards tests)

chain/pool/src/lib.rs Outdated Show resolved Hide resolved
chain/pool/src/lib.rs Show resolved Hide resolved
chain/pool/src/lib.rs Show resolved Hide resolved
@evgenykuzyakov
Copy link
Collaborator Author

@evgenykuzyakov evgenykuzyakov commented Nov 8, 2019

More documentation including detailed example: nearprotocol/nomicon#8

/// 4. If a new entry is needed for a new key, inserts the current entry to the next map.
/// 5. Pulls the latest the transaction from the current entry.
/// 6. If the current entry becomes empty, sets it to None.
impl<'a> DrainingIterator<'a> {
Copy link
Member

@nearmax nearmax Nov 12, 2019

Choose a reason for hiding this comment

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

I suggest we separate DrainingIterator into two iterators: one that iterates over the groups and another one that iterates over the elements within a group. Then both of them can actually implement Iterator trait.

Then instead of passing a boolean on each call of next we can have two nested loops: the outer loop iterates over the groups while the inner iterates over the transactions within the given group, and it only makes a step when the previous transaction was invalid.

Copy link
Member

@nearmax nearmax left a comment

I suggest to rewrite it as two iterators. You can follow this snippet of code that I wrote here: https://gist.github.com/rust-play/0d94d18e16533337380977691a98c7fb

Copy link
Member

@nearmax nearmax left a comment

Here is a version that uses references only: https://gist.github.com/rust-play/0d42b23799a7859d540a8858bd1c3bec

@evgenykuzyakov
Copy link
Collaborator Author

@evgenykuzyakov evgenykuzyakov commented Nov 13, 2019

Thanks. Closing this, see #1657 for changes

@bowenwang1996 bowenwang1996 deleted the tx-limiting branch Apr 27, 2020
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.

None yet

2 participants