-
Notifications
You must be signed in to change notification settings - Fork 107
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those results look really great. Looking forward to see how this behaves in a prod env
for the record, this reverts cce8a97 |
i want a bit more time to dig into this |
@Dieterbe the memory leak is a pretty big issue for production systems. I dont see any reason why this change should be blocked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the benchmarks on cce8a97 I don't see that there were any meaningful gains. Unless I'm reading the benchmarks wrong I'd say go ahead and merge this, especially since it's just going back to previously-working code.
ok that's fair. especially since we now track time we block on the queue. PS @robert-milan please make it a habit to describe the problem and the solution in either the commit message or in the PR (if you put it in the former, it'll automatically go into the PR description as well if there's only 1 commin in the PR). personally i'm a fan of putting it in both. |
so basically:
tradeoffs:
we choose not do synthetic benchmarks to look into these trade-offs. with some luck our prod enviroments are not too noisy so we'll be able to tell from those. |
might help with #1057 or #1058
for testing and results see: https://gist.github.com/robert-milan/20d87f10456b23f297c9eb9e7d730190