Robert/addr memory leak #8717
Robert/addr memory leak #8717
Conversation
|
BTW: we could add same thing for |
Codecov Report
@@ Coverage Diff @@
## master #8717 +/- ##
==========================================
- Coverage 61.41% 61.37% -0.04%
==========================================
Files 674 671 -3
Lines 38399 38336 -63
==========================================
- Hits 23581 23529 -52
- Misses 12335 12348 +13
+ Partials 2483 2459 -24
|
|
|
||
| aa2 := AccAddress{} | ||
| return bytes.Equal(aa.Bytes(), aa2.Bytes()) | ||
| return aa == nil || len(aa) == 0 |
types/address.go
Outdated
| var addMu sync.Mutex | ||
| var addrStrMemoize = make(map[string]string) | ||
| var addrStrMemoize, _ = simplelru.NewLRU(1000, nil) |
There was a problem hiding this comment.
This initialization should be in an init() function, so that we can check the error returned and panic() if it is != nil.
There was a problem hiding this comment.
I was following the current design. That being said I agree with you - I will use init.
There was a problem hiding this comment.
Thank you for this change @robert-zaremba! My apologies for the late reply, I was swamped with a couple of things.
A few things:
a) Please make the []byte->string conversion zero cost and not incur allocations
b) Please revert to using defers, the Go compiler made defer dirt cheap as of Go1.13 in 2019 so these are no longer required, please see https://go.googlesource.com/proposal/+/refs/heads/master/design/34481-opencoded-defers.md which was also publicized for that release
c) To get a really good reason to benchmark the LRU just remember that even if each Bech32 address is 90 characters at most as per https://wiki.trezor.io/Bech32#:~:text=A%20Bech32%20string%20is%20at,The%20human%2Dreadable%20part.&text=The%20separator%2C%20which%20is%20always%20%221%22, to consume 1GiB of RAM, that would need to process ~12M Bech32 addresses per (1<<30)/90, am not sure how often this will happen
d) the consideration of the LRU's contention should be checked and this can be done by writing a benchmark with say 1006 (>LRU_SIZE) bech32 values, run the benchmark before and after this change, and then run benchstat before.txt after.txt
Thank you!
types/address.go
Outdated
| addrMu.Lock() | ||
| addrCache.Add(key, "") | ||
| addrMu.Unlock() |
There was a problem hiding this comment.
There is no need to add this, I'd move all this into the defer as it was before.
There was a problem hiding this comment.
Defer is doing extra allocation ;)
If we won't add a lock here, the race detector will complain.
types/address_bench_test.go
Outdated
| var str string | ||
| for i := 0; i < b.N; i++ { | ||
| str = aa.String() | ||
| } | ||
| require.NotEmpty(b, str) |
There was a problem hiding this comment.
To better get a good benchmark in which code won't be optimized away, use a global variable that's an interface e.g.
var sink, revert interface{}
...
for i := 0; i < b.N; i++ {
result := ...
sink = result
}
if sink == nil {
b.Fatal("benchmark never ran")
}
sink = revertThere was a problem hiding this comment.
You mean a variable outside of any function? We are not using it in other benchmarks.
There was a problem hiding this comment.
Well just because it wasn't done correctly before doesn't mean we shouldn't do it correctly, I certainly have submitted many other benchmarks where we set to a global sink and check that.
There was a problem hiding this comment.
I checked it locally - it doesn't change anything. Are you sure we need to trick the benchmark code?
My only objection is that we make the whole thing less readable and more complex without any observable effect.
There was a problem hiding this comment.
I am sure about code at times getting optimized away and that being the only way to block that. Humbly, for a bit of clarity, I work on the Go project, across almost all the packages and I am bringing this advice from the many years of working on benchmarking related code :-)
There was a problem hiding this comment.
I see. But why to obfuscate more if there is no difference (I verified it)? The require.NoEmpty already assures that value must not be ignored.
|
We can make more sophisticated benchmarks later (feel free to create a task for that). |
There was a problem hiding this comment.
Thank you for the next round of review, @robert-zaremba!
We can make more sophisticated benchmarks later (feel free to create a task for that).
Am not sure what you mean by "sophisticated benchmarks", we need to get conclusive data on the performance and results. I did describe above how to do it. This change is adding in an LRU, replacing a global cache, whose size will only matter if we hit millions of accounts. If the performance of the LRU is worse than for the global map at scale, then we perhaps have other problems.
For benchmarks, after you run them before and after, please use benchstathttps://pkg.go.dev/golang.org/x/tools/cmd/benchstat to give a summary of the comparisons otherwise humans can't tell what changed easily. Also to ensure you get good consistent runs, please use -count=5 or even more, to like -count=20.
The benchmark that I had suggested earlier on will give us conclusive results on the impacts and changes plus what performance improvements or adverse effects that this change brings.
Thanks for the PR.
|
@odeke-em - adding more benchmarks is going out of scope and we should have a good real world scenario here. We can do it in other issue. The intention of this PR was to remove the memory leak. I think it's better to have this update then rolling back the cache. I suggest to create a new issue with further benchmarks. I added one more, non trivial address to the benchmark. |
|
@odeke-em your lock on this PR is still active. Please comment if there is something wrong in this PR. Adding more benchmarks is, imho, out of scope - we would need to go into use-cases / scenario bench-marking to bring bore valuable information. |
|
Thanks for the ping, Robert! I’ll remove my review to avoid blocking you but I definitely think that without a proper and conclusive benchmark, we might not get the right results, as the point of using an LRU was to counteract hogging up memory that won’t get evicted. If the cost of using an LRU is worse that’s a problem. |
The benchmark (which extensively uses cache) shows that's not worse. Moreover the new code uses less allocations - this is due to avoiding |
robert-zaremba
added
the
A:automerge
|
The race-detector tests appears to begin failing with the merge of this pr. Maybe it is due to something else, but thought I'd mention it here |
|
@colin-axner What exactly and Where do you see the test failing?
It's hard to digest the output of The only interesting thing I found is the following warning: This access is OK - because we are fine with a concurrent read, even if it's done during the read - we write data only once, and it never depends on what we read. I can add a mutex there if we want to mute that warning. What do you think? |
|
Ah, I got a little confused by not looking at the full logs I also see this (and similar errors in a couple other places): My main concern is having the race-detector become stuck on always failing. I never check the logs, so I imagine it could be dangerous to have a test which always fails. If someone does introduce a race condition, it could go unnoticed I have little context on this pr, so will defer to the decisions of others here. Maybe the other |
|
Yeah, the logs are huuuuge. I will add a check and let's see what will happen. |
|
Oh wow - why was mergify allowed to merge something whose tests are not all green in the first place? @marbar3778 can we do something to prevent this from happening again? |
|
I think I know why. The EDIT: I fixed the branch configuration |
|
It should be noted that I don't think any pr's can be merged until the race detector is fixed |
The branch configuration is now fixed, so PRs will be blocked until the race is fixed. Does that work for you? |
|
It is fine with me, but it might be blocking a 0.42.0 final release. I'm hoping to remove IBC #8735 this week (so it is included in 0.42). That pr is just waiting on approval of #8624 which I guess now will wait on a race-detector fix Overall though, I am most certainly in favor of fixing tests before merging more pr's and making the problem worse. Just hope we can do so soon |
|
PR: #8767 |
Description
In #8694 cache for
AccAddress.String()was added as a simple map without control and estimates how that map will grow. This leads to unmanageable memory growth, which could put a node under a risk.In this PR,
Address.EmptyfunctionBefore:
After:
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes