Skip to content

Commit 60990f4

Browse files
committed
DPL Analysis: use unsafe cursor when reserving
There is no point in using the slow boundary checking / buffer resizing cursor when we reserve, since it's reasonable to expect that the memory will be there.
1 parent 48fa303 commit 60990f4

2 files changed

Lines changed: 46 additions & 0 deletions

File tree

Framework/Core/include/Framework/AnalysisHelpers.h

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include "Framework/DataAllocator.h"
1717
#include "Framework/IndexBuilderHelpers.h"
1818
#include "Framework/InputSpec.h"
19+
#include "Framework/Logger.h"
1920
#include "Framework/OutputObjHeader.h"
2021
#include "Framework/OutputRef.h"
2122
#include "Framework/OutputSpec.h"
@@ -527,6 +528,9 @@ struct WritingCursor {
527528
mBuilder = std::move(builder);
528529
cursor = std::move(FFL(mBuilder->cursor<persistent_table_t>()));
529530
mCount = -1;
531+
// Back to the safe, bounds-checked cursor: no reservation to validate until
532+
// reserve() is called again for this timeframe.
533+
mReserved = -1;
530534
return true;
531535
}
532536

@@ -537,13 +541,30 @@ struct WritingCursor {
537541

538542
/// reserve @a size rows when filling, so that we do not
539543
/// spend time reallocating the buffers.
544+
/// Switches the internal cursor to UnsafeAppend (no capacity check),
545+
/// which is safe because we just reserved enough space.
540546
void reserve(int64_t size)
541547
{
542548
mBuilder->reserve(typename persistent_table_t::column_types{}, size);
549+
mReserved = size;
550+
cursor = std::move(FFL(mBuilder->template unsafeCursor<persistent_table_t>()));
543551
}
544552

545553
void release()
546554
{
555+
// Called once per timeframe, when the table is finalized. If reserve() was
556+
// used (switching to UnsafeAppend, which skips per-row bounds checks), make
557+
// sure we did not write past what we reserved: mCount + 1 is the number of
558+
// rows actually filled, mReserved the capacity we requested. Overrunning it
559+
// is silent memory corruption of the arrow buffers, so we fail hard here,
560+
// before the (corrupt) table is serialized downstream. mReserved < 0 means
561+
// reserve() was not called and the safe cursor was used: nothing to check.
562+
if (mReserved >= 0 && mCount + 1 > mReserved) {
563+
LOG(fatal) << "Table '" << outputSpec.binding.value << "': filled " << (mCount + 1)
564+
<< " rows after reserve(" << mReserved
565+
<< "). UnsafeAppend overran the reserved buffer — reserve() must request "
566+
"at least as many rows as are filled.";
567+
}
547568
mBuilder.release();
548569
}
549570

@@ -567,6 +588,10 @@ struct WritingCursor {
567588
/// able to do all-columns methods like reserve.
568589
LifetimeHolder<TableBuilder> mBuilder = nullptr;
569590
int64_t mCount = -1;
591+
/// Number of rows reserved via reserve() (which switches to UnsafeAppend);
592+
/// -1 when reserve() was never called. Used by the destructor to detect an
593+
/// UnsafeAppend overrun.
594+
int64_t mReserved = -1;
570595
};
571596

572597
/// Helper to define output for a Table

Framework/Core/include/Framework/TableBuilder.h

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -715,6 +715,18 @@ class TableBuilder
715715
};
716716
}
717717

718+
/// Same as persist, but uses UnsafeAppend (no capacity check).
719+
/// Only safe after reserve() has been called with the correct size.
720+
template <typename ARG0, typename... ARGS>
721+
requires(sizeof...(ARGS) > 0) || ShouldNotDeconstruct<ARG0>
722+
auto unsafePersist()
723+
{
724+
using FillTuple = std::tuple<typename BuilderMaker<ARG0>::FillType, typename BuilderMaker<ARGS>::FillType...>;
725+
return [holders = mHolders](unsigned int /*slot*/, typename BuilderMaker<ARG0>::FillType arg, typename BuilderMaker<ARGS>::FillType... args) -> void {
726+
TableBuilderHelpers::unsafeAppend(*(HoldersTupleIndexed<ARG0, ARGS...>*)holders, std::forward_as_tuple(arg, args...));
727+
};
728+
}
729+
718730
// Same as above, but starting from a o2::soa::Table, which has all the
719731
// information already available.
720732
template <typename T>
@@ -725,6 +737,15 @@ class TableBuilder
725737
}(typename T::table_t::persistent_columns_t{});
726738
}
727739

740+
/// Same as cursor(), but uses UnsafeAppend. Only safe after reserve().
741+
template <typename T>
742+
auto unsafeCursor()
743+
{
744+
return [this]<typename... Cs>(pack<Cs...>) {
745+
return this->template unsafePersist<typename Cs::type...>();
746+
}(typename T::table_t::persistent_columns_t{});
747+
}
748+
728749
template <typename... Cs>
729750
auto cursor(framework::pack<Cs...>)
730751
{

0 commit comments

Comments
 (0)