Given the benchmarks it seems like moving away from gogo is not feasible with the current protobuf schema. I've only now looked at the remote write 2.0 branch, and that might fix the issues with label allocations. Any reason samples are still encoded as a list of pairs? Using two arrays would both reduce the number of objects and allow using varint encoding for timestamps.
On Friday, February 16, 2024 at 1:31:31 PM UTC+1 Mirco wrote: > 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 mircod...@gmail.com 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 mircod...@gmail.com >>>> 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 callu...@gmail.com >>>>>>> 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 < >>>>>>>>> austin...@gmail.com> 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 < >>>>>>>>>> fbra...@gmail.com> 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 < >>>>>>>>>>> austin...@gmail.com> 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 prometheus-developers+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/prometheus-developers/6140ab54-6bd6-40b0-af84-1ba899f88efbn%40googlegroups.com.
