search

zombocom / derailed_benchmarks

Go faster, off the Rails - Benchmarks for your whole Rails app
2.94k stars 138 forks source link

Add `perf:heap_diff` tool #193

Closed BuonOmo closed 3 years ago

BuonOmo commented 3 years ago

Hi,

Thanks for the great tool, we just cleared out a significant memory leak in our app ! However, to do so we also needed a diff between heaps for a better view of what is retained. Since you mention SamSaffron in some parts of your repositories (for instance in heapy's README), I guessed this would be an ok addition.

I've tried to use a similar style and design as other tasks, please tell me if you need any change.

Articles I've cited seems great to me to, but please tell me if you prefer any other resource (which I may discover in the process :smile:)

schneems commented 3 years ago

Looks great. It needs a changelog entry. Also I'm not 100% on the name. At the end of the day what the user wants to do is diff the heaps. That they take 3 snapshots is kind of an implementation detail. What do you think of making it perf:heap_diff ? Or maybe perf:heap_diff_retained?

BuonOmo commented 3 years ago

@schneems thanks for the name I was definitely looking for a better one. No real opinion on which one is better.. I'd say since we already log article references, we can say perf:heap_diff is enough, user should read more to know what they are doing anyway.

BuonOmo commented 3 years ago

Renaming and changelog entry done :)

We may soon write an article on the exact purpose of this PR in our quest to detect a memory leak on sinatra (hence as well the dead_end PR!), if that is the case would it be ok to send another PR referencing this article here ?

schneems commented 3 years ago

if that is the case would it be ok to send another PR referencing this article here ?

Sure. Let me know. Also post to reddit /r/ruby too!

schneems commented 3 years ago

2.1.0 is released with your change. Thanks for the work!