I tried benchmarking BenchmarkSampleSend and the results seem awful, from ~150000 B/op and ~250 allocs/op to ~1400000 B/op to ~170000 allocs/op. Profiling the code, it seems like labelsToLabelsProto is the offender, as it allocates a *prompb.Label per label. I experimented with using a map[string]string instead, but it seems like the results aren't that much different.
One potential solution to this - which would require a relatively big refactor - would be to replace repeated Label labels with the raw byte representation that is held inside of labels_stringlabels, and switch from an array of Sample to two arrays of Timestamp and Value. Just doing the former seems to speed up the benchmarks by 30% (while allocations are still huge because of the *prompb.Sample). On Friday, February 16, 2024 at 10:03:57 AM UTC+1 Bryan Boreham wrote: > Do you have one for sending remote-write data? The one you linked > is RemoteWritehandler which is for receiving. > > From what I've seen elsewhere, Proto3 structs have extra data members > which I expect to show significantly increased memory allocation. > Unfortunately the remote-write structure with a struct for every > name-value pair and all labels repeated is maximally bad for this. > > Bryan > > On Wednesday 14 February 2024 at 18:32:28 UTC [email protected] wrote: > >> Would the already present benchmarks in the code be sufficient? >> >> If so, here are the remote read >> <https://github.com/mircodz/go-proto-bench/blob/main/benchmarks/rr_baseline_csproto> >> , remote write >> <https://github.com/mircodz/go-proto-bench/blob/main/benchmarks/rw_baseline_csproto>, >> >> and a cheeky remote read with pooling for snappy decompression >> <https://github.com/mircodz/go-proto-bench/blob/main/benchmarks/rw_baseline_csproto_pooling> >> comparisons >> (ran with -tags stringlabels), along side the raw results in the same >> directory. >> The remote read tests don't look great, but I might have missed something >> inside of the code. >> >> Note: I took the liberty to use 10 labels for both tests >> <https://github.com/prometheus/prometheus/commit/7080c6ac8cc4175d79a1fd047cb17f0976f57130> >> . >> >> On Wednesday, February 14, 2024 at 10:18:30 AM UTC+1 Bryan Boreham wrote: >> >>> No need to apologise! >>> >>> Do you have benchmarks using the Prometheus remote-read and remote-write >>> protobufs ? >>> >>> On Wednesday 14 February 2024 at 08:08:37 UTC [email protected] wrote: >>> >>>> Hi all, sorry to disrupt this discussion. >>>> >>>> Before stumbling upon this thread, I had worked on a separate fork >>>> <https://github.com/prometheus/prometheus/compare/main...mircodz:prometheus:deprecate-gogo> >>>> >>>> to deprecate gogo in favor of csproto, as compiling it using >>>> enableunsafedecode=true seems >>>> <https://github.com/mircodz/go-proto-bench> to give performance even >>>> better than vtproto. (Note, I have only compared the performance of >>>> csproto and vtproto to the official proto generator, and not gogo). >>>> As of now the branch compiles and passes all tests, but I haven't gone >>>> through the code to check for possible optimizations that could arise from >>>> migrating away from gogo. >>>> Would you be interested in a pull request? As mentioned above, this >>>> would be also a good opportunity to cleanup the proto generation code >>>> using >>>> buf. >>>> >>>> P.S.: This would depend on a change in prometheus/client_model, but >>>> would allow removing the duplicate proto definition in the repository. >>>> >>>> King Regards, >>>> Mirco De Zorzi. >>>> On Monday, February 5, 2024 at 10:58:17 AM UTC+1 Bartłomiej Płotka >>>> wrote: >>>> >>>>> Issue for reference: >>>>> https://github.com/prometheus/prometheus/issues/11908 >>>>> >>>>> Kind Regards, >>>>> Bartek Płotka >>>>> >>>>> On Saturday, February 3, 2024 at 12:56:09 PM UTC Bartłomiej Płotka >>>>> wrote: >>>>> >>>>>> We did a bit of testing for remote write 2.0 work (e.g. here >>>>>> <https://github.com/bwplotka/go-proto-bench>) for gogo vs different >>>>>> plugins, and vtproto is the most promising even with more pointers. >>>>>> >>>>>> We have to get rid of nullables, yes (more pointers, pore separate >>>>>> objects on heap, generally more allocs), but even for our current remote >>>>>> write (especially with interning) there is literally not many slices >>>>>> (with >>>>>> many elements) that use custom types. And even if there are (e.g. >>>>>> []*TimeSeries) those objects might be worth to keep separate on the >>>>>> heap. >>>>>> This is also what protobuf direction will be, given the vision of >>>>>> "opaque >>>>>> API" (ability to load/allocate/ parts of proto message in a lazy way). >>>>>> >>>>>> Furthermore we hit a roadblock a bit, as a apparently "optional >>>>>> <https://github.com/gogo/protobuf/issues/713>" proto3 option does >>>>>> not work with proto. This makes it maybe even more worth doing. (e.g. >>>>>> PRW >>>>>> 2.0 optional timestamp int64 would not be able to have valid value of 0 >>>>>> etc). >>>>>> >>>>>> I think I would consider doing this work this summer, perhaps as a GSoC >>>>>> mentorship >>>>>> <https://github.com/cncf/mentoring/blob/main/programs/summerofcode/2024.md>. >>>>>> >>>>>> Anyone would like to mentor/co-mentor that with me or Callum? (: >>>>>> >>>>>> Kind Regards, >>>>>> Bartek Plotka >>>>>> >>>>>> On Wednesday, November 29, 2023 at 2:38:14 AM UTC [email protected] >>>>>> wrote: >>>>>> >>>>>>> As part of all the remote write proto changes I've been working on I >>>>>>> tried out moving us off of gogoproto, cherry picking Austin's original >>>>>>> changes into a new branch off of the current main branch. >>>>>>> >>>>>>> As Tom mentioned, the main reason for using gogoproto is that >>>>>>> `repeated SomeMessageType = n;` fields within messages are generated as >>>>>>> slices of concrete types rather than slices of pointers, which makes it >>>>>>> much easier to write code that avoids extra memory allocations. From >>>>>>> what >>>>>>> I've hacked together, we can get similar (or potentially better) >>>>>>> performance using vtproto and their pooling feature, but it's going to >>>>>>> be a >>>>>>> big refactoring effort. >>>>>>> >>>>>>> It might, however, be worth it. It looks to me like even with >>>>>>> slightly more allocations the proto marshalling is faster and the >>>>>>> marshalled message is smaller. I'll push what I have later this week >>>>>>> when >>>>>>> I'm more confident it's working correctly. >>>>>>> >>>>>>> On Thursday, July 8, 2021 at 2:42:41 AM UTC-7 Frederic Branczyk >>>>>>> wrote: >>>>>>> >>>>>>>> I think I'd be most useful to rebase, and create a PR from this, >>>>>>>> then we can see whether tests pass and we can run prombench (although >>>>>>>> I >>>>>>>> don't think there are any perf tests that involve the proto parts). >>>>>>>> Then we >>>>>>>> can discuss on there and figure out where to take this. >>>>>>>> >>>>>>>> Thank you so much for the work you have already put into this! >>>>>>>> >>>>>>>> On Mon, 21 Jun 2021 at 19:53, Austin Cawley-Edwards < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> I've updated my branch ( >>>>>>>>> https://github.com/austince/prometheus/tree/feat/drop-gogo) to >>>>>>>>> use both the vitess plugin and the buf tool, which indeed fit very >>>>>>>>> nicely >>>>>>>>> together. >>>>>>>>> >>>>>>>>> I've only updated the code enough for it to compile, have not >>>>>>>>> investigated the semantic differences. This is likely the furthest >>>>>>>>> I'll be >>>>>>>>> able to take this for a bit, so feedback and playing around are >>>>>>>>> welcome and >>>>>>>>> appreciated if this is where we'd like protobuf in Prometheus to go >>>>>>>>> :) >>>>>>>>> >>>>>>>>> Best, >>>>>>>>> Austin >>>>>>>>> >>>>>>>>> On Thu, Jun 17, 2021 at 12:56 PM Frederic Branczyk < >>>>>>>>> [email protected]> wrote: >>>>>>>>> >>>>>>>>>> I have heard great thing, but haven’t used it. Wrongfully thought >>>>>>>>>> that they are mutually exclusive but turns out they are actually >>>>>>>>>> complementary: >>>>>>>>>> https://twitter.com/fredbrancz/status/1405192828049838080?s=21 >>>>>>>>>> >>>>>>>>>> We should probably do an investigation of the combination. >>>>>>>>>> >>>>>>>>>> On Thu 17. Jun 2021 at 18:26, Austin Cawley-Edwards < >>>>>>>>>> [email protected]> wrote: >>>>>>>>>> >>>>>>>>>>> Just saw this on the CNCF blog as well, seems like a promising >>>>>>>>>>> library. >>>>>>>>>>> >>>>>>>>>>> Tangentially, have you heard of https://github.com/bufbuild/buf? >>>>>>>>>>> It seems much nicer than compiling with shell scripts and protoc. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- You received this message because you are subscribed to the Google Groups "Prometheus Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/prometheus-developers/905a998d-998b-4058-a6c9-112c30c9ee59n%40googlegroups.com.
