reuse ValuerEval objects #10414
reuse ValuerEval objects #10414
Conversation
|
At first glance it seems good. We're doing a release very soon though so I'm going to circle back to this later and double check if it's ok to merge. |
|
Seems reasonable. In terms of actual memory allocation load, it's basically gonna be invisible compared to the interfaces anyway. :P |
|
@seebs we will look at this for the next release. Can you remove your changes to the |
seebs
force-pushed
the
seebs/valuerReuse
branch
from
8847bab to
64ed3a9
Compare
|
Updated with the changelog thing dropped. This doesn't apply to master, I think because of code reorganization, but I haven't looked very closely. |
|
I'll take care of rebasing it if needed. Thanks for the help. I'll be looking at this tomorrow so it will likely be merged then. |
|
Thanks! I'm not convinced it's fully thought-through; there might be a much better design lurking. This was mostly picked as a low-impact way to drop a ton of alloc/gc load without substantially changing anything. |
tsdb/engine/tsm1/iterator.gen.go
Outdated
| @@ -223,6 +224,17 @@ func newFloatIterator(name string, tags query.Tags, opt query.IteratorOptions, c | |||
| itr.conds.names = condNames | |||
| itr.conds.curs = conds | |||
|
|
|||
| // note: if opt.Condition is nil, itr.m can be nil. | |||
There was a problem hiding this comment.
You can remove this comment. Passing nil to the MapValuer should be fine regardless. Assigning to it would cause a panic, but that's not relevant to the valuer.
There was a problem hiding this comment.
Good point, that hadn't occurred to me.
There was a problem hiding this comment.
Looks good. Remove the one comment I noted was unnecessary and I'll merge this. If @e-dard gets to it before me, he can also merge it.
Scanner objects and iterators often need a ValuerEval. This object is created, often with a function call, and has at least one interface in it, so it allocates storage. Then it's dropped again right away. The only part of it that might be subject to change is usually a map. While the map's contents change over time, the actual map doesn't change for the lifetime of the object. So, in both iterators and scanners, stash the ValuerEval and continue reusing it. On a query returning a fair number of data points, this produces a small (<5% in practice) improvement in observed performance, visible as a significant reduction in time spent in runtime (mallocgc, newobject, etcetera). The performance improvement isn't big, but it's reasonably easy to evaluate it and establish that it's a safe change to make. Signed-off-by: seebs <seebs@seebs.net>
seebs
force-pushed
the
seebs/valuerReuse
branch
from
64ed3a9 to
5525240
Compare
Scanner objects and iterators often need a ValuerEval. This
object is created, often with a function call, and has at
least one interface in it, so it allocates storage. Then it's
dropped again right away. The only part of it that might be
subject to change is usually a map. While the map's contents
change over time, the actual map doesn't change for the
lifetime of the object.
So, in both iterators and scanners, stash the ValuerEval
and continue reusing it. On a query returning a fair number
of data points, this produces a small (<5% in practice)
improvement in observed performance, visible as a significant
reduction in time spent in runtime (mallocgc, newobject,
etcetera).
The performance improvement isn't big, but it's reasonably
easy to evaluate it and establish that it's a safe change
to make.
Signed-off-by: seebs seebs@seebs.net
Required for all non-trivial PRs
Required only if applicable
You can erase any checkboxes below this note if they are not applicable to your Pull Request.
[none apply]
Notes:
interface{}being slung around.