Process batch serialise concurrently #3877

pocockn · 2023-06-03T20:35:46Z

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

process serialization in parallel

Fixes #3760

Type of change

Please delete options that are not relevant.

[] Bug fix (non-breaking change which fixes an issue)
[] New feature (non-breaking change which adds functionality)
Code refactor or improvement
[] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
[] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

[] make test
[] fullsync
[] Other test (please specify)

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

[] My code follows the style guidelines of this project
[] I have performed a self-review of my code
[] I have commented my code, particularly in hard-to-understand areas
[] I have made corresponding changes to the documentation
[] My changes generate no new warnings
[] I have added tests that prove my fix is effective or that my feature works
[] New and existing unit tests pass locally with my changes
[] Any dependent changes have been merged and published in downstream modules

* upstream/master: (24 commits) account type with zero init nonce (iotexproject#3387) [api] Separate Server and Server Handler (iotexproject#3485) [ioctl] Build hdwallet derive command line into new ioctl (iotexproject#3418) [ioctl] Build hdwallet create command line into new ioctl (iotexproject#3470) [makefile] add go mod tidy (iotexproject#3471) [api] update chain metrics (iotexproject#3484) remove config.EVMNetworkID() (iotexproject#3460) [filedao] remove checkMasterChainDBFile() (iotexproject#3463) [api] add crashlog (iotexproject#3456) [api] Move generateBlockMeta to grpcserver.go (iotexproject#3303) [ioctl] Build action hash command line into new ioctl (iotexproject#3425) [ioctl] Build hdwallet export command line into new ioctl (iotexproject#3423) [ioctl] Refactor nodereward command in new ioctl (iotexproject#3416) [ioctl] Cleanup TestNewNodeDelegateCmd (iotexproject#3421) [blockchain] Remove BoltDBDaoOption (iotexproject#3465) remove InMemDaoOption (iotexproject#3464) [action] add evm london test (iotexproject#3402) [ioctl] create main for ioctl/newcmd (iotexproject#3296) [ioctl] Build block bucket command line into new ioctl (iotexproject#3386) [ioctl] Build hdwallet import command line into new ioctl (iotexproject#3419) ...

* upstream/master: (45 commits) Task: Get config cmd (iotexproject#3552) [ioctl] fix Errors unhandled (iotexproject#3567) fix dir permission and file inclusion (iotexproject#3566) [test] Disable workingset cache in the benchmark test (iotexproject#3558) [pkg] fix deferring unsafe method "Close" on type "*os.File" (iotexproject#3548) [action] Refactor handleTransfer() (iotexproject#3557) Add MinVersion in tls.Config (iotexproject#3562) [ioctl] Modify file permission as 0600 (iotexproject#3563) [httputil] add ReadHeaderTimeout (iotexproject#3550) [staking] unexport namespace (iotexproject#3551) move chanid metrics to chainservice (iotexproject#3544) [ioctl] fix log entries created from user input (iotexproject#3546) add log in rolldposctx (iotexproject#3553) fix uncontrolled data used in path expression (iotexproject#3547) [api] impl. TestGrpcServer_GetServerMeta (iotexproject#3559) [ioctl] Build action command line into new ioctl (iotexproject#3472) fix potential file inclusion via variable (iotexproject#3549) [ioctl] Incorrect conversion between integer types (iotexproject#3522) [action] fix incorrect conversion between integer types (iotexproject#3545) [test] fix TestLoadBlockchainfromDB (iotexproject#3521) ...

db/batch/batch_impl.go

db/batch/batch_impl_test.go

dustinxie · 2023-06-06T01:42:02Z

db/batch/batch_impl.go

+	bytes := make([]byte, 0)
+	for itemBytes := range bytesChan {
+		bytes = append(bytes, itemBytes...)
+	}


in general this is fine, but probably won't work for our case. Because the total bytes will be hashed to serve as a digest of the batch's content, which is used later in validation. So the bytes has to be in fixed order, and working parallel breaks this order

I see, does this mean the PR is no longer needed? Thanks

I think you should ensure that the order of writing bytes matches the order in the writeQueue. As for the order of concurrent serialization in between does not matter .

@envestcc modified it so the writing of the bytes matches the order 👍

Liuhaai · 2023-06-06T18:19:24Z

using a pre-allocated array could enable the processing in parallel (e.g. blks code in https://github.com/iotexproject/iotex-core/blob/master/blockindex/bloomfilterindexer_test.go#L294-L321)

LuckyPigeon · 2023-06-07T03:40:29Z

@pocockn
I've encountered similar problem before, pls check #3180.
IMO, you could use

errgroup to validate if any of your go routine returns error.
Like @Liuhaai mentioned, add a pre-allocated array should fix the order problem.

db/batch/batch_impl.go

codecov · 2023-06-13T14:42:59Z

Codecov Report

Merging #3877 (42add3b) into master (e1f0636) will increase coverage by 0.18%.
The diff coverage is 67.00%.

❗ Current head 42add3b differs from pull request most recent head 5716c32. Consider uploading reports for the commit 5716c32 to get more accurate results

@@            Coverage Diff             @@
##           master    #3877      +/-   ##
==========================================
+ Coverage   75.38%   75.56%   +0.18%     
==========================================
  Files         303      328      +25     
  Lines       25923    27805    +1882     
==========================================
+ Hits        19541    21012    +1471     
- Misses       5360     5717     +357     
- Partials     1022     1076      +54

Impacted Files	Coverage Δ
action/protocol/execution/evm/evm.go	`43.52% <0.00%> (-2.95%)`	⬇️
action/protocol/execution/evm/evmstatedbadapter.go	`66.77% <ø> (ø)`
action/protocol/poll/consortium.go	`0.00% <0.00%> (ø)`
action/protocol/poll/staking_committee.go	`43.85% <0.00%> (ø)`
...tion/protocol/staking/contractstake_bucket_type.go	`0.00% <0.00%> (ø)`
api/grpcserver.go	`86.40% <0.00%> (ø)`
api/loglistener.go	`25.00% <0.00%> (ø)`
api/web3server_marshal.go	`93.21% <ø> (ø)`
api/websocket.go	`5.17% <0.00%> (ø)`
blockchain/config.go	`74.54% <ø> (ø)`
... and 51 more

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Liuhaai · 2023-06-13T17:33:00Z

db/batch/batch_impl.go

-		} else {
-			bytes = append(bytes, wi.Serialize()...)
-		}
+	// 1. Digest could be replaced by merkle root if we need proof


b.mutex.Lock() defer b.mutex.Unlock()

should the mtx be kept?

Hey @Liuhaai , @envestcc said it's not needed as the concurrent writes now happen on different indices within the slice

https://stackoverflow.com/questions/42238425/golang-mutex-ranging-over-shared-array-in-goroutines

could write an unit test to verify the concurrency problem

mtx is for b.writeQueue

Added the mtx back @Liuhaai

does the Unlock can be called earlier? It seems like that it can be placed just before wg.Wait().

but it seems that this is unrelated to the current issue. It may not improve the serialization process. Perhaps it can be discussed in a new issue.

sonarcloud · 2023-06-20T13:43:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

Liuhaai · 2023-06-21T05:19:46Z

lgtm

pocockn and others added 7 commits June 9, 2022 15:18

Task: Return error from keep alive methods within httputil pkg

996573e

Merge branch 'master' into master

3b3504e

Merge branch 'master' into master

e705bba

Merge branch 'master' into master

3a272b4

Run serialise concurrently within batch impl

59eb5a0

pocockn requested review from CoderZhi, dustinxie and Liuhaai as code owners June 3, 2023 20:35

pocockn added 2 commits June 3, 2023 21:50

tweak test

d117115

Fix data race

088bc8e

pocockn requested review from envestcc and a team as code owners June 3, 2023 22:01

Buffer channel to avoid deadlok

37e9fb0

envestcc requested changes Jun 5, 2023

View reviewed changes

db/batch/batch_impl.go Show resolved Hide resolved

db/batch/batch_impl_test.go Outdated Show resolved Hide resolved

Fix wg done not being caleld

3d00c43

dustinxie reviewed Jun 6, 2023

View reviewed changes

Fix tests and return the serialised byte array in the same order

4b10b27

envestcc reviewed Jun 12, 2023

View reviewed changes

db/batch/batch_impl.go Outdated Show resolved Hide resolved

pocockn added 4 commits June 12, 2023 11:37

Remove the mutex lock as we're accessing the slice at different indices

7b1f120

Merge branch 'master' into master

796d7c9

Remove actpool code that snuck i

e8d4733

Remove comment

aeddae8

Liuhaai reviewed Jun 13, 2023

View reviewed changes

pocockn added 2 commits June 16, 2023 13:41

Merge branch 'master' into master

6e7fa46

Merge branch 'master' into master

41c26b7

Add back in mutex for the b.writeQueue

5716c32

Liuhaai approved these changes Jun 21, 2023

View reviewed changes

envestcc approved these changes Jun 21, 2023

View reviewed changes

Liuhaai merged commit 1abbc44 into iotexproject:master Jun 21, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process batch serialise concurrently #3877

Process batch serialise concurrently #3877

pocockn commented Jun 3, 2023

dustinxie Jun 6, 2023

pocockn Jun 6, 2023

envestcc Jun 6, 2023

pocockn Jun 11, 2023

Liuhaai commented Jun 6, 2023

LuckyPigeon commented Jun 7, 2023 •

edited

Loading

codecov bot commented Jun 13, 2023 •

edited

Loading

Liuhaai Jun 13, 2023

pocockn Jun 14, 2023

pocockn Jun 16, 2023

envestcc Jun 16, 2023

Liuhaai Jun 16, 2023

pocockn Jun 20, 2023

envestcc Jun 21, 2023

sonarcloud bot commented Jun 20, 2023

Liuhaai commented Jun 21, 2023

Process batch serialise concurrently #3877

Process batch serialise concurrently #3877

Conversation

pocockn commented Jun 3, 2023

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Liuhaai commented Jun 6, 2023

LuckyPigeon commented Jun 7, 2023 • edited Loading

codecov bot commented Jun 13, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 20, 2023

Liuhaai commented Jun 21, 2023

LuckyPigeon commented Jun 7, 2023 •

edited

Loading

codecov bot commented Jun 13, 2023 •

edited

Loading