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

Mon Feb 25 17:18:35 PST 2019

dberris added a comment.

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.

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.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58621