[Consensus] Cheap message comparison #1712
Conversation
|
Please add a test case. If we ever add a field, we might forget to update the |
| }) | ||
|
|
||
| // Better | ||
| b.Run("cmp", func(b *testing.B) { |
There was a problem hiding this comment.
I have no idea where "gotest.tools/assert/cmp" is from. is the import related to this improvement?
There was a problem hiding this comment.
Yes, but I forgot to remove its comparison since it cannot be used in production env. Now it's removed.
| }) | ||
|
|
||
| // Best | ||
| b.Run("own", func(b *testing.B) { |
There was a problem hiding this comment.
Could you make some test codes that verify this improvement does not make any problems with consensus? Thank you in advance.
There was a problem hiding this comment.
Also, I did not get the meaning of "own" here. Does it mean it is your own code?
There was a problem hiding this comment.
- TC added.
ownindicates a newly added comparison method. Any suggestion for renaming?
There was a problem hiding this comment.
how about "precompiledEqual"? or "staticDeepEqual"?
There was a problem hiding this comment.
Hmm.., I think just a EuqalImpl seems enough. The suggested names seem overly named than it does actually.
|
@blukat29, Thanks. added. |
|
@hyunsooda This is too late to be added. I think this can be merged after v1.10 is released. Is it okay to you? |
|
@hyunsooda Can you rebase against the latest dev? Then I think linter can pass. |
the confirmation of the same output with the previous one
|
@hyunsooda Could you rebase against the latest dev please? |
|
Apologies for the delay. I will proceed with the next steps if this change is deemed feasible and desirable for merging. If not, please inform me, and I will close it. |
adeb45e
|
@hyunsooda Could you resolve the test-linter? |
Proposed changes
Consensus messages are basically compared two times in one round if no round change happens. The number of comparisons depends on how many committees participate at each round. For 4 CNs, total of 8 times compared. This PR proposes a cheap comparison. This change was inspired by @hqjang-pepper 's presentation.
cc. @jiseongnoh , @yoomee1313, @mckim19
Types of changes
Please put an x in the boxes related to your change.
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.
$ make test)Related issues
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...