feat: concurrent checkTx #49

jinsan-line · 2021-01-11T06:16:56Z

Related with: https://github.com/line/link/issues/1151, Finschia/ostracon#160

Description

To optimize performance, we need to increase concurrency. As a first step for it, I implement concurrent checkTx. The key change is to remove app.mtx.Lock() from the abci local client. An application, as an abci server, is better to protect itself from concurrency than an abci client. W/ current implementation, the abci local client protects an abci server but it decreases concurrency.

CONTRACT:

Now, an application should protect itself from concurrent checkTx as an abci server that means it should be thread safe.
We'll also increase concurrency for other abci methods as much as possible in the future.

What should application protect?

In cosmos-sdk application, it should protect accounts from the concurrency.

Motivation and context

How has this been tested?

make test
make test-cover

Checklist:

I followed the contributing guidelines.
I have updated the documentation accordingly.
I have added tests to cover my changes.

egonspace · 2021-01-12T00:45:37Z

x/auth/ante/accountlock.go

+		tail3 := addr[len(addr)-3:]
+		tail := append([]byte{0}, tail3...)
+
+		addrKey := binary.BigEndian.Uint32(tail)


addrMtx has so many redundant instances not being used because a character of addr cannot cover all of 0~255.
I think we should use decoded bytes for mutex hash.

_, data, _ := bech32.Decode(addr) // one character of data has a value 0~31 (5 bits) addrKey := (int(data[0]) << 10) | (int(data[1]) << 5) | int(data[2]) // it may be 0~32767

And I think 32k mutexs are enough.

And for reference, I think it is not good to use the last 6 bytes of Addr as hash because it is checksum.

@egonspace

addrMtx has so many redundant instances not being used because a character of addr cannot cover all of 0~255.
I think we should use decoded bytes for mutex hash.

AFAIK, AccAddress is not encoded address but is decoded address.

And I think 32k mutexs are enough.

Please could you share the reason? I think we need a metric for conflict ratio. I'll investigate it w/ dashboard. But, as a basic approach rule, I think the team is already aligned to utilize memory to optimize performance because it is cheapest among cpu, memory, and storage. A Mutex is 8bytes so addrMtx uses 32MiB. I think 32MiB is not matter but conflict ratio is matter in terms of performance.

And for reference, I think it is not good to use the last 6 bytes of Addr as hash because it is checksum.

AFAIK, AccAddress is decoded and doesn't have checksum. And also I think checksum is better for key than sampling in terms of distribution and conflict possibility. I'll think it more. Thanks for inspiring me.

Oh, I'm sorry I didn't know AccAddress was already decoded form. And then, it seems that code has no problem.
But isn't the memory allocated as mutex 128MB? Doesn't 1 << 24 mean 16777216? So 8*16777216/1024/1024 = 128.
Of course, I agree with the statement that it would be good to accumulate collision statistics and then judge them.

I don't think it's good to use checksum as a hash because I don't know how to calculate the checksum and I don't know if the variance is as even as hash.

@egonspace

Thanks, I mistook simple calculation. addrMtx uses 128MiB.

the number of mutex: 1 << 24 = 2 ^ 24 = (2 ^ 4) * (2 ^ 10) * (2 ^ 10) = 16M

mutex size: 8bytes

8bytes * 16M = 128MiB.

I still believe it's not too big but, as I said, we should monitor with a practical experiment.

I don't think it's good to use checksum as a hash because I don't know how to calculate the checksum and I don't know if the variance is as even as hash.

As I said, I'm still thinking it but don't have personal conclusion yet.

@egonspace

The number of mutex

I set up sampleBytes as 2. Now, the number of mutex is 64k so accMtx uses 512KiB. (ec00f66)

With monitoring, if I found the conflict ratio is too high, I'll increase it at that time.

AccKey

AccAddress might be random number so I get personal conclusion that just sampling is enough.

If you have any idea, please let me know.
Thanks,

egonspace

LGTM

wetcod

LGTM

baseapp/accountlock.go

kukugi

LGTM 👍

* feat: implement new abci, `BeginRecheckTx()` and `EndRecheckTx()` * test: fix tests * refactor: decompose checkTx & runTx * chore: protect app.checkState w/ RWMutex for simulate * chore: remove unused var * feat: account lock decorator * chore: skip AccountLockDecorator if not checkTx * chore: bump up tendermint * chore: revise accountlock position * chore: accountlock_test * chore: revise accountlock covers `cache.Write()` * chore: revise `sampleBytes` to `2` * fix: test according to `sampleBytes` * chore: revise `getUniqSortedAddressKey()` and add `getAddressKey()` * chore: revise `how to sort` not to use `reflection` * chore: bump up tendermint * test: check `sorted` in `TestGetUniqSortedAddressKey()` * chore: move `accountLock` from `anteTx()` to `checkTx()` # Conflicts: # baseapp/abci.go # baseapp/baseapp.go # baseapp/baseapp_test.go # baseapp/helpers.go # go.mod # go.sum # x/bank/bench_test.go # x/mock/test_utils.go

* chore: bump up ostracon, iavl and tm-db * feat: concurrent checkTx (#49) * feat: implement new abci, `BeginRecheckTx()` and `EndRecheckTx()` * test: fix tests * refactor: decompose checkTx & runTx * chore: protect app.checkState w/ RWMutex for simulate * chore: remove unused var * feat: account lock decorator * chore: skip AccountLockDecorator if not checkTx * chore: bump up tendermint * chore: revise accountlock position * chore: accountlock_test * chore: revise accountlock covers `cache.Write()` * chore: revise `sampleBytes` to `2` * fix: test according to `sampleBytes` * chore: revise `getUniqSortedAddressKey()` and add `getAddressKey()` * chore: revise `how to sort` not to use `reflection` * chore: bump up tendermint * test: check `sorted` in `TestGetUniqSortedAddressKey()` * chore: move `accountLock` from `anteTx()` to `checkTx()` # Conflicts: # baseapp/abci.go # baseapp/baseapp.go # baseapp/baseapp_test.go # baseapp/helpers.go # go.mod # go.sum # x/bank/bench_test.go # x/mock/test_utils.go * fix: make it buildable * fix: tests * fix: gasWanted & gasUsed are always `0` (#51) * fix: gasWanted & gasUsed is always `0` * chore: error log for general panic # Conflicts: # baseapp/baseapp.go

jinsan-line added 8 commits January 8, 2021 15:27

feat: implement new abci, BeginRecheckTx() and EndRecheckTx()

a772b5a

test: fix tests

8279d3c

refactor: decompose checkTx & runTx

f2202f5

chore: protect app.checkState w/ RWMutex for simulate

9197ca4

chore: remove unused var

096d2fc

feat: account lock decorator

0d59278

chore: skip AccountLockDecorator if not checkTx

34b4821

chore: bump up tendermint

29d3229

jinsan-line self-assigned this Jan 11, 2021

jinsan-line marked this pull request as draft January 11, 2021 06:56

jinsan-line added 2 commits January 11, 2021 17:19

chore: revise accountlock position

bb253dd

chore: accountlock_test

284cf08

jinsan-line requested review from kukugi, wetcod, tnasu, egonspace and kfangw January 11, 2021 08:53

jinsan-line marked this pull request as ready for review January 11, 2021 08:54

jinsan-line marked this pull request as draft January 11, 2021 10:07

egonspace reviewed Jan 12, 2021

View reviewed changes

jinsan-line added 2 commits January 12, 2021 16:26

chore: revise accountlock covers cache.Write()

440cabe

chore: revise sampleBytes to 2

ec00f66

egonspace approved these changes Jan 12, 2021

View reviewed changes

fix: test according to sampleBytes

c578a53

jinsan-line marked this pull request as ready for review January 12, 2021 08:24

wetcod approved these changes Jan 12, 2021

View reviewed changes

jinsan-line added 2 commits January 12, 2021 22:03

chore: revise getUniqSortedAddressKey() and add getAddressKey()

ba50dbc

chore: revise how to sort not to use reflection

64239e3

wetcod reviewed Jan 12, 2021

View reviewed changes

baseapp/accountlock.go Show resolved Hide resolved

kukugi approved these changes Jan 13, 2021

View reviewed changes

wetcod approved these changes Jan 13, 2021

View reviewed changes

tnasu approved these changes Jan 13, 2021

View reviewed changes

jinsan-line added 3 commits January 13, 2021 17:35

chore: bump up tendermint

ae1ad65

test: check sorted in TestGetUniqSortedAddressKey()

f190415

chore: move accountLock from anteTx() to checkTx()

1df011c

jinsan-line merged commit 2982988 into Finschia:feat/perf Jan 15, 2021

jinsan-line deleted the concurrent-checktx branch January 15, 2021 04:58

This was referenced Jan 22, 2021

fix: gasWanted & gasUsed are always 0 #51

Merged

feat: concurrent recheckTx #52

Merged

feat: concurrent deliverTx #53

Merged

jinsan-line mentioned this pull request Apr 22, 2021

feat: concurrent checkTx #141

Merged

9 tasks

This was referenced Apr 22, 2021

feat: implement validateGasWanted() (#48) #142

Merged

feat: concurrent recheckTx (#52) #155

Merged

egonspace unassigned jinsan-line Jun 28, 2021

tnasu mentioned this pull request Jul 13, 2023

Backport tendermint-v0.34.20 into main Finschia/ostracon#642

Merged

Mdaiki0730 mentioned this pull request Sep 6, 2023

Make rdk compatible with tendermint #1115

Merged

5 tasks

0Tech mentioned this pull request Dec 22, 2023

[Epic]: triage the PRs from Finschia/finschia-sdk Finschia/cosmos-sdk#1

Open

96 tasks

0Tech mentioned this pull request Jan 12, 2024

Triage finschia-sdk#49 Finschia/cosmos-sdk#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: concurrent checkTx #49

feat: concurrent checkTx #49

jinsan-line commented Jan 11, 2021 •

edited

Loading

egonspace Jan 12, 2021 •

edited

Loading

egonspace Jan 12, 2021

jinsan-line Jan 12, 2021 •

edited

Loading

egonspace Jan 12, 2021

jinsan-line Jan 12, 2021 •

edited

Loading

jinsan-line Jan 12, 2021 •

edited

Loading

egonspace left a comment

wetcod left a comment

kukugi left a comment

feat: concurrent checkTx #49

feat: concurrent checkTx #49

Conversation

jinsan-line commented Jan 11, 2021 • edited Loading

Description

Motivation and context

How has this been tested?

Checklist:

egonspace Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

egonspace Jan 12, 2021

Choose a reason for hiding this comment

jinsan-line Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

egonspace Jan 12, 2021

Choose a reason for hiding this comment

jinsan-line Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

jinsan-line Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

egonspace left a comment

Choose a reason for hiding this comment

wetcod left a comment

Choose a reason for hiding this comment

kukugi left a comment

Choose a reason for hiding this comment

jinsan-line commented Jan 11, 2021 •

edited

Loading

egonspace Jan 12, 2021 •

edited

Loading

jinsan-line Jan 12, 2021 •

edited

Loading

jinsan-line Jan 12, 2021 •

edited

Loading

jinsan-line Jan 12, 2021 •

edited

Loading