Page MenuHomePhabricator

[XRay][compiler-rt] Add APIs for processing logs in memory
ClosedPublic

Authored by dberris on Feb 19 2018, 4:45 PM.

Details

Summary

This change adds APIs to allow logging implementations to provide a
function for iterating through in-memory buffers (if they hold in-memory
buffers) and a way for users to generically deal with these buffers
in-process. These APIs are:

  • xray_log_set_buffer_iterator(...) and xray_log_remove_buffer_iterator(): installs and removes an iterator function that takes an XRayBuffer and yields the next one.
  • __xray_log_process_buffers(...): takes a function pointer that can take a mode identifier (string) and an XRayBuffer to process this data as they see fit.

The intent is to have the FDR mode implementation's buffers be
available through this __xray_log_process_buffers(...) API, so that
they can be streamed from memory instead of flushed to disk (useful for
getting the data to a network, or doing in-process analysis).

Basic mode logging will not support this mechanism as it's designed to
write the data mostly to disk.

Future implementations will may depend on this API as well, to allow for
programmatically working through the XRay buffers exposed to the
users in some fashion.

Diff Detail

Repository
rL LLVM

Event Timeline

dberris created this revision.Feb 19 2018, 4:45 PM
dberris added a subscriber: echristo.

Adding @echristo as additional reviewer.

kpw added a comment.Feb 26 2018, 10:34 AM

Mostly looks good, but I think you should consider making the buffer iterator interface part of XRayLogImpl or some other mechanism to associate it with a logging mode.

Iterating the buffers is going to be tightly coupled to the impl that already manages the buffers.

compiler-rt/include/xray/xray_log_interface.h
241 ↗(On Diff #134992)

I would change "currently installed" to "currently selected". Multiple nodes may be registered (installed) but only one selected.

276 ↗(On Diff #134992)

Nit: XRayBUffer -> XRayBuffer

309–320 ↗(On Diff #134992)

Should __xray_log_set_buffer_iterator be part of XRayLogImpl? A suitable default would simply return the empty XRayBuffer.

Since exposing the buffers is tightly coupled to the requirements of the handler, it is odd that it must be registered separately and can't be pinned to a mode.

compiler-rt/lib/xray/xray_log_interface.cc
166 ↗(On Diff #134992)

Isn't it enough to check Buffer.Data != nullptr?

If for some reason, an implementation maintains valid pointers to zero size buffers, that implementation is crummy, but there's no good reason to skip the rest of the buffers in the iterator.

compiler-rt/test/xray/TestCases/Linux/logging-modes.cc
78 ↗(On Diff #134992)

Why not be more specific and assert it's equal to 1 exactly?

dberris updated this revision to Diff 136946.Mar 4 2018, 5:19 PM
dberris marked 4 inline comments as done.
  • fixup: Rebase and address comments
In D43495#1019473, @kpw wrote:

Mostly looks good, but I think you should consider making the buffer iterator interface part of XRayLogImpl or some other mechanism to associate it with a logging mode.

Iterating the buffers is going to be tightly coupled to the impl that already manages the buffers.

That makes sense, but I'm trying really hard to not change the ABI for the XRay APIs here -- changing the XRayLogImpl struct will be an ABI break, and we don't have an ABI versioning check implemented yet.

In the meantime, the suggestion is to make the installation of the log processing handler(s) be done in the initialisation of the log implementations. This would allow log implementations to tightly control what the functionality for iterating and processing the data would be.

compiler-rt/include/xray/xray_log_interface.h
309–320 ↗(On Diff #134992)

It should be, but changing it would be an ABI break. We're not quite ready to do that yet at this point.

compiler-rt/lib/xray/xray_log_interface.cc
166 ↗(On Diff #134992)

Good catch, thanks, Keith!

kpw accepted this revision.Mar 6 2018, 5:54 PM

Good point about preserving the size and structure of XRayLogImpl for ABI compatibility. Piper monorepo has lulled me into a false sense of security!

This revision is now accepted and ready to land.Mar 6 2018, 5:54 PM
This revision was automatically updated to reflect the committed changes.