fix hash name bug (#479) #482
Conversation
Signed-off-by: 923048992@qq.com <923048992@qq.com>
Signed-off-by: 923048992@qq.com <923048992@qq.com>
| // HashName converts a string to a uint32 integer as the key of bpf map | ||
| type HashName struct { | ||
| numToStr map[uint32]string | ||
| // records its tombstone number given a string | ||
| tombstones map[string]uint32 |
There was a problem hiding this comment.
This is kind of mem leak. Our requirement is:
- a str should be allocated with a consistent value
- after it deleted, and then at some time recreated, the value can be different.
- It's better to be consistent for the case that kmesh daemon restart, which seems difficult with this implement. Maybe we can persist the maps somewhere.
There was a problem hiding this comment.
2. after it deleted, and then at some time recreated, the value can be different.
Requirement 1 means for a str like "foo", if we call StrToNum we will get number 123. Then at anytime we StrToNum("foo") should always get 123? Even if collision happened?
Can you give an example about requirement 2?
Thanks 😊
There was a problem hiding this comment.
In the original implemention, there exists another problem.
If A collides with B.
If StrToNum(A) = 123, then StrToNum(B) = 124
Then we delete A, we will delete numToStr[123].
Then we delete B, we will also delete numToStr[123], cause Delete will call StrToNum, where StrToNum(B) will be 123, if A is deleted(since there is no collision any more). In this case, we will leave numToStr[124] = B, which is also a kind of mem leak.
There was a problem hiding this comment.
after it deleted, and then at some time recreated, the value can be different.
say foo at t0 is allcated 1234, then at t1, it is deleted. In our case, all the info is deleted from bpf map.
Maybe t2, foo is recreated, then we can allocate it to 2345. It need to be consistent during its lifecycle.
There was a problem hiding this comment.
I got it. We need to persist the numtostr map in case kmesh restarts.
There was a problem hiding this comment.
In the original implemention, there exists another problem. If A collides with B. If StrToNum(A) = 123, then StrToNum(B) = 124 Then we delete A, we will delete numToStr[123]. Then we delete B, we will also delete numToStr[123], cause Delete will call StrToNum, where StrToNum(B) will be 123, if A is deleted(since there is no collision any more). In this case, we will leave numToStr[124] = B, which is also a kind of mem leak.
to solve this problem, we will still need the tombstone marker
There was a problem hiding this comment.
We only need to store string that cause collision.
For example, if A collides with B.
We call StrToNum(A), then StrToNum(B).
A got "123", B got "124"
We only need to persist (B, 124). Since after restart, we call StrToNum(B) we can get StrToNum(B) = 124 from persisted map. And StrToNum(A) = 123 according to hash value.
| } else if h.numToStr[num] == str { | ||
| h.strToNum[str] = num | ||
| // Create a new item here, should flush | ||
| if err := h.flush(); err != nil { |
There was a problem hiding this comment.
Can we write delta data? Flush all seems very expensive
There was a problem hiding this comment.
Sure. What about appending it?
Like:
Before:
{123: "foo"}After(remove '}' and add new item, and add '}' again, instead of write all data):
{123: "foo", 124: "bar"}There was a problem hiding this comment.
maybe yaml is simpler
foo: 123
bar: 124
There was a problem hiding this comment.
maybe yaml is simpler
foo: 123 bar: 124
right, that's a better idea.
| if num, exits := h.strToNum[str]; exits { | ||
| return num | ||
| } | ||
|
|
||
| // Using linear probing to solve hash conflicts | ||
| for num = hash.Sum32(); num < math.MaxUint32; num++ { |
There was a problem hiding this comment.
not related to your code。
If num overflows, should we iterate from 0
There was a problem hiding this comment.
I think it's necessary. It should be a ring.
Signed-off-by: 923048992@qq.com <923048992@qq.com>
Signed-off-by: 923048992@qq.com <923048992@qq.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #479
Special notes for your reviewer:
Does this PR introduce a user-facing change?: