[PATCH] D58621: [XRay][tools] Pack XRayRecord - reduce memory footprint by a third. (RFC)

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

In D58621#1410322 <https://reviews.llvm.org/D58621#1410322>, @lebedev.ri wrote:

> In D58621#1409962 <https://reviews.llvm.org/D58621#1409962>, @dberris wrote:
>
> > In D58621#1409715 <https://reviews.llvm.org/D58621#1409715>, @lebedev.ri wrote:
> >
> > > In D58621#1409704 <https://reviews.llvm.org/D58621#1409704>, @dberris wrote:
> > >
> > > > > This is a RFC because of the uint8_t CPU change.
> > > > >  That chance needs discussing.
> > > >
> > > > So, this is an accident of history, which should be changed, but to the other direction. I've learned some time ago that it turns out there are some platforms that can have enough CPU IDs which can't be represented by a uint8_t. For future-proofing, we really should change this to be larger (`uint16_t`) and change basic mode to store 16-bit CPU IDs.
> > >
> > >
> > > Boo :)
> >
> >
> > Boo indeed. :)
> >
> > > Unfortunately that won't just cost that one extra byte, it will have ripple effect on the padding in this struct.
> > >  I'm not sure as to exact numbers.
> >
> > I like the idea of reducing top-line memory requirements, but it shouldn't be at the cost of functionality. The current state is a bug that we're only using 8 bits for the CPU ID.
>
>
> No, i totally understand.
>  That is why i said it's RFC and is only valid if it is actually
>  only ever 8 bits. (which it is, but only due to the bug elsewhere)
>  I think this is still worth it even with 16-bit CPU, i'll take a look.
>
> > Now, an alternative here is to migrate the Basic Mode implementation to use a more compact log record (i.e. using the FDR mode format), and use a different converter approach, one that doesn't require reconstituting the whole `Trace` consisting of `XRayRecord` instances. The FDR log loading libraries/framework allow us to do this now (see `llvm-xray fdr-dump`). This is a more intensive project but one that isn't terribly hard to accomplish. If you'd like to take that on, I'd be happy to review patches going in that direction instead (really the only difference between the current basic mode implementation and the FDR mode implementation is that, basic mode threads returning a buffer to the queue will write out the contents before returning the buffer to the central buffer queue). In that process we can migrate FDR mode to use 16-bit CPU IDs.
>
> So it's basically three co-dependent steps:
>
> 1. Teach `xray convert` to also work on FDR input
> 2. Afterwards, switch the compiler-rt X-Ray basic log to output FDR log format.
> 3. Finally, fix the truncation of CPU id in the compiler-rt xray code


Step 1 is not strictly necessary, we already somewhat already do this although indirectly (through the `Trace` type) in `llvm-xray convert`. We can then later switch to stream-processing the FDR mode logs when converting, which will be very similar to what the `fdr-dump` subcommand already does.

Step 2 and 3 can happen in a single change.

> //I'm// presently not constrained by the `XRayRecord` memory footprint any more,
>  so i'm not sure how much effort i want to spent here..

This I understand too. :)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58621/new/

https://reviews.llvm.org/D58621