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

Make handoff to record-stream thread w/ ArrayBlockingQueue#offer not LinkedBlockingQueue#put #1547

Merged

Conversation

tinker-michaelj
Copy link
Collaborator

Sonar passed w/ no (actual) new code smells here.

Related issue(s):
Closes #1500

Summary of the change:

  • Create a NonBlockingHandoff that effectively puts an ArrayBlockingQueue between the handleTransaction thread and the recordStream thread.
  • This protects handleTransaction from blocking on the LinkedBlockingQueue#put used by the RecordStreamManager threads, which from profiling appears almost 200% slower than ArrayBlockingQueue#offer.

Note: This PR also adds a memo field to the TxnAccessor to eliminate a curiously expensive call to the gRPC TransactionBody.getMemo() method.

tinker-michaelj and others added 30 commits May 22, 2021 10:51
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
tinker-michaelj added 19 commits May 30, 2021 11:34
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #1547 (95049e3) into master-plus-sdk-0.15.0 (f6d4e7b) will increase coverage by 0.00%.
The diff coverage is 94.28%.

Impacted file tree graph

@@                    Coverage Diff                    @@
##             master-plus-sdk-0.15.0    #1547   +/-   ##
=========================================================
  Coverage                     91.53%   91.53%           
- Complexity                     5880     5892   +12     
=========================================================
  Files                           462      463    +1     
  Lines                         17014    17044   +30     
  Branches                       1778     1780    +2     
=========================================================
+ Hits                          15574    15602   +28     
  Misses                          986      986           
- Partials                        454      456    +2     
Impacted Files Coverage Δ
...in/java/com/hedera/services/utils/TxnAccessor.java 0.00% <ø> (ø)
...vices/legacy/services/state/AwareProcessLogic.java 34.34% <50.00%> (-0.36%) ⬇️
...a/com/hedera/services/context/ServicesContext.java 93.52% <66.66%> (-0.11%) ⬇️
...edera/services/state/expiry/ExpiringCreations.java 92.59% <100.00%> (ø)
...com/hedera/services/stream/NonBlockingHandoff.java 100.00% <100.00%> (ø)
...a/com/hedera/services/utils/SignedTxnAccessor.java 97.95% <100.00%> (+0.08%) ⬆️

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 f6d4e7b...95049e3. Read the comment docs.

Copy link
Contributor

@ljianghedera ljianghedera left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@abhishek-hedera abhishek-hedera left a comment

Choose a reason for hiding this comment

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

LGTM

@tinker-michaelj tinker-michaelj merged commit 1cbe488 into master-plus-sdk-0.15.0 Jun 7, 2021
@tinker-michaelj tinker-michaelj deleted the 01500-M0150-CreateNonBlockingHandoff branch June 7, 2021 16:29
Copy link
Contributor

@qnswirlds qnswirlds left a comment

Choose a reason for hiding this comment

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

Provided a few places to improve in a different clean-up PR.

//setup:
// setup:
nonBlockingHandoff = mock(NonBlockingHandoff.class);
given(nonBlockingHandoff.offer(any())).willReturn(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should test with returning false a few times before returning true.

@@ -59,6 +59,7 @@
import static org.hamcrest.collection.IsIterableContainingInOrder.contains;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.eq;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: added but not used

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