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.

Sign up for GitHub

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

Jump to bottom

x/staking: lazily get consensus key address #9264

Merged
merged 2 commits into from
May 5, 2021
Merged

x/staking: lazily get consensus key address #9264

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f485e31
x/staking: lazily get consensus key address
May 5, 2021
2692f9b
Merge branch 'master' into cuonglm/issue-9263
May 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions x/staking/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,17 +237,22 @@ func validateGenesisStateValidators(validators []types.Validator) error {
if err != nil {
return err
}
consAddr, err := val.GetConsAddr()
if err != nil {
return err
}

strKey := string(consPk.Bytes())

if _, ok := addrMap[strKey]; ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can even further avoid the []byte->string allocation by using
_, ok := addrMap[string(consPk.Bytes())]
if ok {
...
}

...

addrMap[string(consPk.Bytes())] = ...
which will mos definitely show an improvement in the hot path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the []byte -> string trick is not applicable, because consPk.Bytes() is a method call, the compiler need to allocate temporary variable for that, which is roughly like:

tmp := consPk.Bytes()
strKey := string(tmp)

Here's the benchmark result (comparing with this PR):

name                            old time/op    new time/op    delta
ValidateGenesis10Validators-8     1.13µs ± 1%    1.09µs ± 0%  -3.46%  (p=0.008 n=5+5)
ValidateGenesis100Validators-8    10.1µs ± 0%    10.3µs ± 1%  +2.90%  (p=0.008 n=5+5)
ValidateGenesis400Validators-8    38.3µs ± 0%    40.5µs ± 0%  +5.76%  (p=0.008 n=5+5)

name                            old alloc/op   new alloc/op   delta
ValidateGenesis10Validators-8       667B ± 0%      667B ± 0%    ~     (all equal)
ValidateGenesis100Validators-8    6.22kB ± 0%    6.22kB ± 0%    ~     (p=0.159 n=5+5)
ValidateGenesis400Validators-8    24.4kB ± 0%    24.4kB ± 0%    ~     (p=0.810 n=5+5)

name                            old allocs/op  new allocs/op  delta
ValidateGenesis10Validators-8       13.0 ± 0%      13.0 ± 0%    ~     (all equal)
ValidateGenesis100Validators-8       105 ± 0%       105 ± 0%    ~     (all equal)
ValidateGenesis400Validators-8       408 ± 0%       408 ± 0%    ~     (all equal)

it actually run slower because we call the method 2 times, which will introduce 2 temp vars.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great point about it being a method call! However at least with a single variable then directly doing the map []->string call should work unless the compiler is getting faulty but I don’t see how doing m[string(bKey)] will make things slower it should eliminate the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@odeke-em Hmm, not sure I got what you mean. What make thing slower because we call consPk.Bytes() 2 times, which will introduce 2 temp vars.

With those temp vars, the m[string([]byte)] optimization is applied already, that's why you can see your suggestion and the PR have the same alloc in benchmark result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you see my second suggestion? Essentially adapt accordingly so:

bKey := consPk.Bytes()
m[string(bKey)]
...
m[string(bKey)]

No way that's not going to not eliminate the 2 []byte->string conversions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With those temp vars, the m[string([]byte)] optimization is applied already, that's why you can see your suggestion and the PR have the same alloc in benchmark result.

I don't believe so fully, you are doing
strKey := string(...)

Then using strKey around, we don't have to incur that conversion at all given that strKey is being used as the key instead of m[string(bKey)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@odeke-em What you do:

bKey := consPk.Bytes()

then do string(bKey) in lookup, is already the same thing with what string(consPk.Bytes()) does. Because the compiler rewrite it to:

tmp := consPk.Bytes()
strKey := string(tmp)

the compiler can realize that tmp isn't modified elsewhere, so the string conversion make no allocation. Here's the benchmark result with your second suggestion:

name                            old time/op    new time/op    delta
ValidateGenesis10Validators-8     1.14µs ± 0%    1.09µs ± 0%   -4.12%  (p=0.008 n=5+5)
ValidateGenesis100Validators-8    10.1µs ± 0%    11.3µs ±15%  +12.02%  (p=0.008 n=5+5)
ValidateGenesis400Validators-8    38.4µs ± 0%    40.5µs ± 1%   +5.42%  (p=0.008 n=5+5)

name                            old alloc/op   new alloc/op   delta
ValidateGenesis10Validators-8       667B ± 0%      667B ± 0%     ~     (all equal)
ValidateGenesis100Validators-8    6.22kB ± 0%    6.22kB ± 0%     ~     (p=1.000 n=5+5)
ValidateGenesis400Validators-8    24.4kB ± 0%    24.4kB ± 0%     ~     (p=0.206 n=5+5)

name                            old allocs/op  new allocs/op  delta
ValidateGenesis10Validators-8       13.0 ± 0%      13.0 ± 0%     ~     (all equal)
ValidateGenesis100Validators-8       105 ± 0%       105 ± 0%     ~     (all equal)
ValidateGenesis400Validators-8       408 ± 0%       408 ± 0%     ~     (all equal)

Copy link
Collaborator

@odeke-em odeke-em May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Tell me then, what does the next m[strKey] down below amount to instead of m[string(bKey)] much down below where there is not an edit? Not a hill am willing to die on and later on can be revised but would be nice to examine all the code paths.

consAddr, err := val.GetConsAddr()
cuonglm marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
return fmt.Errorf("duplicate validator in genesis state: moniker %v, address %v", val.Description.Moniker, consAddr)
}

if val.Jailed && val.IsBonded() {
consAddr, err := val.GetConsAddr()
if err != nil {
return err
}
return fmt.Errorf("validator is bonded and jailed in genesis state: moniker %v, address %v", val.Description.Moniker, consAddr)
}

Expand Down