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 commented Nov 5, 2019

@nearmax Does this make sense so far?

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Collaborator Author

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

This comment has been minimized.

Copy link
Collaborator

nearmax commented Nov 6, 2019

@nearmax Does this make sense so far?

So this PR is rewrites the code in #1591 ?

@evgenykuzyakov

This comment has been minimized.

Copy link
Collaborator Author

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 {

This comment has been minimized.

Copy link
@nearmax

nearmax Nov 6, 2019

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

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Nov 6, 2019

Author Collaborator

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) {

This comment has been minimized.

Copy link
@nearmax

nearmax Nov 6, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Nov 7, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
Collaborator Author

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> {

This comment has been minimized.

Copy link
@nearmax

nearmax Nov 12, 2019

Collaborator

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
Collaborator

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
Collaborator

nearmax left a comment

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

@evgenykuzyakov

This comment has been minimized.

Copy link
Collaborator Author

evgenykuzyakov commented Nov 13, 2019

Thanks. Closing this, see #1657 for changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.