[chore] Reuse the JSON writer to reduce memory allocations #27806
Conversation
atoulme
force-pushed
the
use_json_writer
branch
from
8cdd351 to
4db7d37
Compare
|
@dmitryax please review when you get a chance |
exporter/splunkhecexporter/client.go
Outdated
| if uint(jsonStream.Buffered()) > c.config.MaxEventSize { | ||
| permanentErrors = append(permanentErrors, fmt.Errorf("event size %d exceeds limit %d", jsonStream.Buffered(), c.config.MaxEventSize)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
I don't think this approach works for metrics. We are not flushing or resetting the stream once this condition is triggered. This means we will be dropping all the events after we have buffered more than c.config.MaxEventSize. This is incorrect because MaxEventSize is supposed to be applied to each individual event.
One way forward I can think of is introducing another field to iterState for "datapoints" and using it in this loop. Having that, we don't need to remove marshalEvent (or whatever we call it) and copy-paste the code. Later we can use it in mapMetricToSplunkEvent as well to avoid redoing translations.
There was a problem hiding this comment.
ok let me drop that code path for now, and we can revisit when we have exporterhelper and a way to keep track of event consumption.
There was a problem hiding this comment.
OK changes made - the optimization is much less important, we get 3% only, but hopefully it helps for other cases.
There was a problem hiding this comment.
Looks like the number of allocations is the same. In some cases, we utilize less memory overall and cpu, but it doesn't seem too much. At the same time, we make the code less readable. I'm not 100% sure if this change is a good improvement, considering a hypothetical error budget
|
Closing ; we can revisit this whole logic once exporterhelper is there. |
Description:
Reuse the JSON writer to reduce memory allocations.
Before:
After:
Benchstat: