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

Refactor Trie hasher using onRoot flag #1840

Merged
merged 3 commits into from May 25, 2023

Conversation

blukat29
Copy link
Contributor

@blukat29 blukat29 commented May 9, 2023

Proposed changes

De-duplicate code in the trie hasher (storage/statedb/hasher.go).

  • Current implementation has two pairs of similar functions:
    • single-threaded version hash() + hashChildren()
    • multi-threaded version hashRoot() + hashChildrenFromRoot()
  • This PR removes the latter. Instead, the onRoot argument switches multi-threading.
    • hashNode(..., onRoot) + hashChildren(..., onRoot) runs multi-threaded if onRoot == true.
      • hash(...) = hashNode(..., onRoot=false)
      • hashRoot(...) = hashNode(..., onRoot=true)

Types of changes

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

Further comments

Benchmark results show that this PR does not affect hashing performance (on Apple M1 Max 10-core)

  • Before PR
     $ go test -run=^$ -bench=BenchmarkHash
     BenchmarkHash-10                 2582688               402.2 ns/op           684 B/op          7 allocs/op
     BenchmarkHashFixedSize/10-10       26340             43323 ns/op           14891 B/op        179 allocs/op
     BenchmarkHashFixedSize/100-10              12986             89688 ns/op           66881 B/op        809 allocs/op
     BenchmarkHashFixedSize/1K-10                2024            618712 ns/op          659172 B/op       7811 allocs/op
     BenchmarkHashFixedSize/10K-10                282           4171724 ns/op         6907710 B/op      79703 allocs/op
     BenchmarkHashFixedSize/100K-10                33          35102691 ns/op        68240330 B/op     794537 allocs/op
    
  • After PR
     BenchmarkHash-10                 3081148               439.9 ns/op           677 B/op          7 allocs/op
     BenchmarkHashFixedSize/10-10       27524             42308 ns/op           14262 B/op        178 allocs/op
     BenchmarkHashFixedSize/100-10              13624             88954 ns/op           66293 B/op        808 allocs/op
     BenchmarkHashFixedSize/1K-10                2050            604276 ns/op          658532 B/op       7810 allocs/op
     BenchmarkHashFixedSize/10K-10                280           4168147 ns/op         6906603 B/op      79701 allocs/op
     BenchmarkHashFixedSize/100K-10                30          38524168 ns/op        68240050 B/op     794539 allocs/op
    
  • (for comparison) After PR, if onRoot == false
     BenchmarkHash-10                  564274              2035 ns/op             628 B/op          7 allocs/op
     BenchmarkHashFixedSize/10-10       41187             28873 ns/op           13213 B/op        155 allocs/op
     BenchmarkHashFixedSize/100-10               8718            134050 ns/op           64321 B/op        772 allocs/op
     BenchmarkHashFixedSize/1K-10                 842           1415162 ns/op          649540 B/op       7742 allocs/op
     BenchmarkHashFixedSize/10K-10                 70          17122263 ns/op         6865380 B/op      79500 allocs/op
     BenchmarkHashFixedSize/100K-10                 7         164345345 ns/op        68196154 B/op     794339 allocs/op	
    

@blukat29 blukat29 self-assigned this May 9, 2023
@blukat29 blukat29 marked this pull request as draft May 10, 2023 03:35
@blukat29 blukat29 changed the title Refactor Trie hasher using parallel flag Refactor Trie hasher using onRoot flag May 11, 2023
@blukat29 blukat29 marked this pull request as ready for review May 12, 2023 00:39
storage/statedb/hasher.go Outdated Show resolved Hide resolved
ethan-kr
ethan-kr previously approved these changes May 16, 2023
Copy link
Contributor

@ethan-kr ethan-kr left a comment

Choose a reason for hiding this comment

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

I made a comment, but there is no disagreement, so I approve it.

Copy link
Contributor

@jeongkyun-oh jeongkyun-oh left a comment

Choose a reason for hiding this comment

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

LGTM

@blukat29
Copy link
Contributor Author

@ethan-kr PTAL :)

@blukat29 blukat29 merged commit 3825a41 into klaytn:dev May 25, 2023
11 checks passed
@blukat29 blukat29 deleted the refactor-trie-hasher branch July 17, 2023 02:06
@JayChoi1736 JayChoi1736 mentioned this pull request Jul 24, 2023
20 tasks
@blukat29 blukat29 mentioned this pull request Sep 20, 2023
9 tasks
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.

None yet

4 participants