This is an archive of the discontinued LLVM Phabricator instance.

[XRay][compiler-rt] Generational Buffer Management
ClosedPublic

Authored by dberris on Oct 7 2018, 10:51 PM.

Details

Summary

This change updates the buffer queue implementation to support using a
generation number to identify the lifetime of buffers. This first part
introduces the notion of the generation number, without changing the way
we handle the buffers yet.

What's missing here is the cleanup of the buffers. Ideally we'll keep
the two most recent generations. We need to ensure that before we do any
writes to the buffers, that we check the generation number(s) first.

Those changes will follow-on from this change.

Depends on D52588.

Diff Detail

Repository
rL LLVM

Event Timeline

dberris created this revision.Oct 7 2018, 10:51 PM
mboerger added inline comments.Oct 8 2018, 4:33 AM
compiler-rt/lib/xray/tests/unit/buffer_queue_test.cc
130 ↗(On Diff #168611)

Why not test B1.Generation > B0.Generation?

137 ↗(On Diff #168611)

Test that the next generation is > B1.Generation even after release?

dberris updated this revision to Diff 169646.Oct 14 2018, 10:24 PM
dberris marked an inline comment as done.

Address some comments by @mboerger.

dberris added inline comments.Oct 14 2018, 10:24 PM
compiler-rt/lib/xray/tests/unit/buffer_queue_test.cc
130 ↗(On Diff #168611)

I don't think exposing that Generation might be increasing monotonically is worth enforcing in the test. Because Generation is unsigned, it can overflow back to 0 and the > property doesn't hold (and it's not meant to).

137 ↗(On Diff #168611)

Good point. I've added a second cycle, just to ensure that we are able to keep doing the generational buffer management in the single test case.

This revision is now accepted and ready to land.Oct 14 2018, 11:04 PM
This revision was automatically updated to reflect the committed changes.
dberris reopened this revision.Oct 21 2018, 9:16 PM

This had been reverted. Reopening now to update.

This revision is now accepted and ready to land.Oct 21 2018, 9:16 PM
dberris updated this revision to Diff 170366.Oct 21 2018, 9:16 PM

Updates to expand the critical region, to include synchronisation on the backing store.

This revision was automatically updated to reflect the committed changes.