8000 Seperate trie commit and hash by hqjang-pepper · Pull Request #1656 · klaytn/klaytn · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Aug 19, 2024. It is now read-only.

Seperate trie commit and hash #1656

Closed

Conversation

hqjang-pepper
Copy link
Contributor
@hqjang-pepper hqjang-pepper commented Nov 2, 2022

Proposed changes

Seperate Trie committer and hasher

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • 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

  • Please leave the issue numbers or links related to this PR here.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@hqjang-pepper hqjang-pepper added the do not merge don't merge this PR yet label Nov 2, 2022
@hqjang-pepper hqjang-pepper marked this pull request as draft November 2, 2022 01:14
@hqjang-pepper hqjang-pepper mentioned this pull request Nov 2, 2022
9 tasks
@hqjang-pepper hqjang-pepper marked this pull request as ready for review November 2, 2022 07:35
@hqjang-pepper hqjang-pepper removed the do not merge don't merge this PR yet label Nov 3, 2022
@hqjang-pepper hqjang-pepper added this to the v1.10.0 (Kore) milestone Nov 3, 2022
@hqjang-pepper
Copy link
Contributor Author
hqjang-pepper commented Nov 11, 2022

Benchmark Hash and commit

  • Hash

Before
BenchmarkHashFixedSize
BenchmarkHashFixedSize/10
BenchmarkHashFixedSize/10-10 	           27526	     43064 ns/op	   14005 B/op	     157 allocs/op
BenchmarkHashFixedSize/100
BenchmarkHashFixedSize/100-10         	   10000	    101392 ns/op	   65680 B/op	     777 allocs/op
BenchmarkHashFixedSize/1K
BenchmarkHashFixedSize/1K-10          	    1190	   1004009 ns/op	  657317 B/op	    7782 allocs/op
BenchmarkHashFixedSize/10K
BenchmarkHashFixedSize/10K-10         	     274	   4429889 ns/op	 6905728 B/op	   79660 allocs/op
BenchmarkHashFixedSize/100K
BenchmarkHashFixedSize/100K-10        	      27	  38932048 ns/op	68239165 B/op	  794500 allocs/op


After
BenchmarkHashFixedSize
BenchmarkHashFixedSize/10
BenchmarkHashFixedSize/10-10 	           37155	     32239 ns/op	   11942 B/op	     135 allocs/op
BenchmarkHashFixedSize/100
BenchmarkHashFixedSize/100-10         	    7766	    153061 ns/op	   57988 B/op	     671 allocs/op
BenchmarkHashFixedSize/1K
BenchmarkHashFixedSize/1K-10          	     798	   1505582 ns/op	  584060 B/op	    6728 allocs/op
BenchmarkHashFixedSize/10K
BenchmarkHashFixedSize/10K-10         	      66	  18371760 ns/op	 6224453 B/op	   69396 allocs/op
BenchmarkHashFixedSize/100K
BenchmarkHashFixedSize/100K-10        	       6	 177527841 ns/op	61796261 B/op	  693042 allocs/op

  • Commit

Before
BenchmarkCommitAfterHashFixedSize
BenchmarkCommitAfterHashFixedSize/10
BenchmarkCommitAfterHashFixedSize/10-10 	           30988	     38790 ns/op	   36083 B/op	     208 allocs/op
BenchmarkCommitAfterHashFixedSize/100
BenchmarkCommitAfterHashFixedSize/100-10         	    8286	    145892 ns/op	  174370 B/op	    1022 allocs/op
BenchmarkCommitAfterHashFixedSize/1K
BenchmarkCommitAfterHashFixedSize/1K-10          	     828	   1485660 ns/op	 1701478 B/op	   10143 allocs/op
BenchmarkCommitAfterHashFixedSize/10K
BenchmarkCommitAfterHashFixedSize/10K-10         	      74	  14636495 ns/op	18696716 B/op	  103985 allocs/op
BenchmarkCommitAfterHashFixedSize/100K
BenchmarkCommitAfterHashFixedSize/100K-10        	       7	 148162095 ns/op	180172019 B/op	 1038887 allocs/op

After
BenchmarkCommitAfterHashFixedSize
BenchmarkCommitAfterHashFixedSize/10
BenchmarkCommitAfterHashFixedSize/10-10 	           60080	     20030 ns/op	   30914 B/op	     189 allocs/op
BenchmarkCommitAfterHashFixedSize/100
BenchmarkCommitAfterHashFixedSize/100-10         	   12500	     96729 ns/op	  151233 B/op	     918 allocs/op
BenchmarkCommitAfterHashFixedSize/1K
BenchmarkCommitAfterHashFixedSize/1K-10          	    1052	   1188825 ns/op	 1473359 B/op	    9108 allocs/op
BenchmarkCommitAfterHashFixedSize/10K
BenchmarkCommitAfterHashFixedSize/10K-10         	      94	  11857340 ns/op	16368251 B/op	   93862 allocs/op
BenchmarkCommitAfterHashFixedSize/100K
BenchmarkCommitAfterHashFixedSize/100K-10        	       9	 121488097 ns/op	157054739 B/op	  937567 allocs/op

@hqjang-pepper
Copy link
Contributor Author

The reason that the execution time of the hash function is longer than expected is because parallel processing is removed from this part of code. This was rolled back in PR #1659, and you can see that the performance improvement occurred in that PR.

@hqjang-pepper hqjang-pepper mentioned this pull request Nov 14, 2022
9 tasks
Comment on lines +237 to +241
// For children in range [0, 15], it's impossible
// to contain valueNode. Only check the 17th child.
if n.Children[16] != nil {
c.onleaf(nil, nil, n.Children[16].(valueNode), hash, 0)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about this? Would you mind to take a look insert method of Trie?
Please let me know if I'm wrong.

_, branch.Children[n.Key[matchlen]], err = t.insert(nil, append(prefix, n.Key[:matchlen+1]...), n.Key[matchlen+1:], n.Val)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think there's no specific problem in here.
Same as the existing onleaf method.

8000
@kjhman21 kjhman21 modified the milestones: v1.10 (Kore), v1.11 (Mantle) Nov 21, 2022
@hqjang-pepper
Copy link
Contributor Author

@kjhman21 Was this PR milestone changed to v1.11?

8000

@kjhman21
Copy link

@hqjang-pepper yes.

@aidan-kwon aidan-kwon marked this pull request as draft July 18, 2023 03:38
@aidan-kwon aidan-kwon removed this from the v1.11.0 milestone Jul 24, 2023
@blukat29
Copy link
Contributor

Won't do in this version. closing.

@blukat29 blukat29 closed this Oct 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0