Skip to content

Commit

Permalink
[ntuple] Introduce and use new function MakeUninitArray
Browse files Browse the repository at this point in the history
With std::make_unique<T[]>, the array elements are value-initialized
which is oftentimes not necessary for buffers. See also the message of
commit 975989d ("Do not memset() a buffer that will be overwritten").
In particular, that commit changed the usage in RPageSinkBuf to allocate
sealed page buffers, which I "simplified" in commit 80018cd. This
should become more clear with the explicit function name.
  • Loading branch information
hahnjo committed Nov 21, 2024
1 parent ae72deb commit 1c94d18
Show file tree
Hide file tree
Showing 8 changed files with 46 additions and 34 deletions.
10 changes: 10 additions & 0 deletions tree/ntuple/v7/inc/ROOT/RNTupleUtil.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,16 @@ auto MakeAliasedSharedPtr(T *rawPtr)
return std::shared_ptr<T>(fgRawPtrCtrlBlock, rawPtr);
}

/// Make an array of default-initialized elements. This is useful for buffers that do not need to be initialized.
///
/// With C++20, this function can be replaced by std::make_unique_for_overwrite<T[]>.
template <typename T>
std::unique_ptr<T[]> MakeUninitArray(std::size_t size)
{
// DO NOT use std::make_unique<T[]>, the array elements are value-initialized!
return std::unique_ptr<T[]>(new T[size]);
}

inline constexpr EColumnType kTestFutureType =
static_cast<EColumnType>(std::numeric_limits<std::underlying_type_t<EColumnType>>::max() - 1);

Expand Down
13 changes: 7 additions & 6 deletions tree/ntuple/v7/src/RColumnElement.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ namespace {

using ROOT::Experimental::EColumnType;
using ROOT::Experimental::Internal::kTestFutureType;
using ROOT::Experimental::Internal::MakeUninitArray;
using ROOT::Experimental::Internal::RColumnElementBase;

template <typename CppT, EColumnType>
Expand Down Expand Up @@ -903,7 +904,7 @@ public:
#if R__LITTLE_ENDIAN == 0
// TODO(gparolini): to avoid this extra allocation we might want to perform byte swapping
// directly in the Pack/UnpackBits functions.
auto bswapped = std::make_unique<float[]>(count);
auto bswapped = MakeUninitArray<float>(count);
CopyBswap<sizeof(float)>(bswapped.get(), src, count);
const auto *srcLe = bswapped.get();
#else
Expand Down Expand Up @@ -936,15 +937,15 @@ public:

// Cast doubles to float before packing them
// TODO(gparolini): avoid this allocation
auto srcFloat = std::make_unique<float[]>(count);
auto srcFloat = MakeUninitArray<float>(count);
const double *srcDouble = reinterpret_cast<const double *>(src);
for (std::size_t i = 0; i < count; ++i)
srcFloat[i] = static_cast<float>(srcDouble[i]);

#if R__LITTLE_ENDIAN == 0
// TODO(gparolini): to avoid this extra allocation we might want to perform byte swapping
// directly in the Pack/UnpackBits functions.
auto bswapped = std::make_unique<float[]>(count);
auto bswapped = MakeUninitArray<float>(count);
CopyBswap<sizeof(float)>(bswapped.get(), srcFloat.get(), count);
const float *srcLe = bswapped.get();
#else
Expand All @@ -960,7 +961,7 @@ public:
R__ASSERT(GetPackedSize(count) == MinBufSize(count, fBitsOnStorage));

// TODO(gparolini): avoid this allocation
auto dstFloat = std::make_unique<float[]>(count);
auto dstFloat = MakeUninitArray<float>(count);
UnpackBits(dstFloat.get(), src, count, sizeof(float), fBitsOnStorage);
#if R__LITTLE_ENDIAN == 0
InPlaceBswap<sizeof(float)>(dstFloat.get(), count);
Expand Down Expand Up @@ -1110,7 +1111,7 @@ public:
using namespace ROOT::Experimental;

// TODO(gparolini): see if we can avoid this allocation
auto quantized = std::make_unique<Quantize::Quantized_t[]>(count);
auto quantized = MakeUninitArray<Quantize::Quantized_t>(count);
assert(fValueRange);
const auto [min, max] = *fValueRange;
const int nOutOfRange =
Expand All @@ -1128,7 +1129,7 @@ public:
using namespace ROOT::Experimental;

// TODO(gparolini): see if we can avoid this allocation
auto quantized = std::make_unique<Quantize::Quantized_t[]>(count);
auto quantized = MakeUninitArray<Quantize::Quantized_t>(count);
assert(fValueRange);
const auto [min, max] = *fValueRange;
Internal::BitPacking::UnpackBits(quantized.get(), src, count, sizeof(Quantize::Quantized_t), fBitsOnStorage);
Expand Down
8 changes: 4 additions & 4 deletions tree/ntuple/v7/src/RMiniFile.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,7 @@ ROOT::Experimental::Internal::RMiniFileReader::GetNTupleProper(std::string_view
}
// The object length can be smaller than the size of RTFNTuple if it comes from a past RNTuple class version,
// or larger than it if it comes from a future RNTuple class version.
auto bufAnchor = std::make_unique<unsigned char[]>(std::max<size_t>(key.fObjLen, sizeof(RTFNTuple)));
auto bufAnchor = MakeUninitArray<unsigned char>(std::max<size_t>(key.fObjLen, sizeof(RTFNTuple)));
RTFNTuple *ntuple = new (bufAnchor.get()) RTFNTuple;

auto objNbytes = key.GetSize() - key.fKeyLen;
Expand Down Expand Up @@ -800,7 +800,7 @@ void ROOT::Experimental::Internal::RMiniFileReader::ReadBuffer(void *buffer, siz
bufCur += nbytesFirstChunk;
nread -= nbytesChunkOffsets;

const auto chunkOffsets = std::make_unique<std::uint64_t[]>(nChunks - 1);
const auto chunkOffsets = MakeUninitArray<std::uint64_t>(nChunks - 1);
memcpy(chunkOffsets.get(), bufCur, nbytesChunkOffsets);

size_t remainingBytes = nbytes - nbytesFirstChunk;
Expand Down Expand Up @@ -1203,7 +1203,7 @@ std::uint64_t ROOT::Experimental::Internal::RNTupleFileWriter::WriteBlob(const v
const uint8_t *chunkData = reinterpret_cast<const uint8_t *>(data) + nbytesFirstChunk;
size_t remainingBytes = nbytes - nbytesFirstChunk;

const auto chunkOffsetsToWrite = std::make_unique<std::uint64_t[]>(nChunks - 1);
const auto chunkOffsetsToWrite = MakeUninitArray<std::uint64_t>(nChunks - 1);
std::uint64_t chunkOffsetIdx = 0;

do {
Expand Down Expand Up @@ -1327,7 +1327,7 @@ void ROOT::Experimental::Internal::RNTupleFileWriter::WriteTFileStreamerInfo()
const auto lenPayload = buffer.Length() - keyLen;

RNTupleCompressor compressor;
auto zipStreamerInfos = std::make_unique<unsigned char[]>(lenPayload);
auto zipStreamerInfos = MakeUninitArray<unsigned char>(lenPayload);
auto szZipStreamerInfos = compressor.Zip(bufPayload, lenPayload, 1, zipStreamerInfos.get());

fFileSimple.WriteKey(zipStreamerInfos.get(), szZipStreamerInfos, lenPayload,
Expand Down
2 changes: 1 addition & 1 deletion tree/ntuple/v7/src/RNTupleMerger.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ void RNTupleMerger::MergeCommonColumns(RClusterPool &clusterPool, DescriptorId_t
if (needsCompressionChange) {
const auto uncompressedSize = srcColElement->GetSize() * sealedPage.GetNElements();
auto &buffer = sealedPageData.fBuffers[pageBufferBaseIdx + pageIdx];
buffer = std::make_unique<std::uint8_t[]>(uncompressedSize + checksumSize);
buffer = MakeUninitArray<std::uint8_t>(uncompressedSize + checksumSize);
RChangeCompressionFunc compressTask{
*srcColElement, *dstColElement, mergeData.fMergeOpts, sealedPage, *fPageAlloc, buffer.get(),
};
Expand Down
4 changes: 2 additions & 2 deletions tree/ntuple/v7/src/RPageSinkBuf.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -153,15 +153,15 @@ void ROOT::Experimental::Internal::RPageSinkBuf::CommitPage(ColumnHandle_t colum
auto &sealedPage = fBufferedColumns.at(colId).RegisterSealedPage();

auto allocateBuf = [&zipItem, maxSealedPageBytes]() {
zipItem.fBuf = std::make_unique<unsigned char[]>(maxSealedPageBytes);
zipItem.fBuf = MakeUninitArray<unsigned char>(maxSealedPageBytes);
R__ASSERT(zipItem.fBuf);
};
auto shrinkSealedPage = [&zipItem, maxSealedPageBytes, &sealedPage]() {
// If the sealed page is smaller than the maximum size (with compression), allocate what is needed and copy the
// sealed page content to save memory.
auto sealedBufferSize = sealedPage.GetBufferSize();
if (sealedBufferSize < maxSealedPageBytes) {
auto buf = std::make_unique<unsigned char[]>(sealedBufferSize);
auto buf = MakeUninitArray<unsigned char>(sealedBufferSize);
memcpy(buf.get(), sealedPage.GetBuffer(), sealedBufferSize);
zipItem.fBuf = std::move(buf);
sealedPage.SetBuffer(zipItem.fBuf.get());
Expand Down
6 changes: 3 additions & 3 deletions tree/ntuple/v7/src/RPageStorage.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -876,7 +876,7 @@ void ROOT::Experimental::Internal::RPagePersistentSink::InitImpl(RNTupleModel &m
UpdateSchema(initialChangeset, 0U);

fSerializationContext = RNTupleSerializer::SerializeHeader(nullptr, descriptor);
auto buffer = std::make_unique<unsigned char[]>(fSerializationContext.GetHeaderSize());
auto buffer = MakeUninitArray<unsigned char>(fSerializationContext.GetHeaderSize());
fSerializationContext = RNTupleSerializer::SerializeHeader(buffer.get(), descriptor);
InitImpl(buffer.get(), fSerializationContext.GetHeaderSize());

Expand Down Expand Up @@ -1108,7 +1108,7 @@ void ROOT::Experimental::Internal::RPagePersistentSink::CommitClusterGroup()
}

auto szPageList = RNTupleSerializer::SerializePageList(nullptr, descriptor, physClusterIDs, fSerializationContext);
auto bufPageList = std::make_unique<unsigned char[]>(szPageList);
auto bufPageList = MakeUninitArray<unsigned char>(szPageList);
RNTupleSerializer::SerializePageList(bufPageList.get(), descriptor, physClusterIDs, fSerializationContext);

const auto clusterGroupId = descriptor.GetNClusterGroups();
Expand Down Expand Up @@ -1148,7 +1148,7 @@ void ROOT::Experimental::Internal::RPagePersistentSink::CommitDatasetImpl()
const auto &descriptor = fDescriptorBuilder.GetDescriptor();

auto szFooter = RNTupleSerializer::SerializeFooter(nullptr, descriptor, fSerializationContext);
auto bufFooter = std::make_unique<unsigned char[]>(szFooter);
auto bufFooter = MakeUninitArray<unsigned char>(szFooter);
RNTupleSerializer::SerializeFooter(bufFooter.get(), descriptor, fSerializationContext);

CommitDatasetImpl(bufFooter.get(), szFooter);
Expand Down
27 changes: 14 additions & 13 deletions tree/ntuple/v7/src/RPageStorageDaos.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ namespace {
using AttributeKey_t = ROOT::Experimental::Internal::RDaosContainer::AttributeKey_t;
using DistributionKey_t = ROOT::Experimental::Internal::RDaosContainer::DistributionKey_t;
using ntuple_index_t = ROOT::Experimental::Internal::ntuple_index_t;
using ROOT::Experimental::Internal::MakeUninitArray;

/// \brief RNTuple page-DAOS mappings
enum EDaosMapping { kOidPerCluster, kOidPerPage };
Expand Down Expand Up @@ -162,7 +163,7 @@ struct RDaosContainerNTupleLocator {
const auto anchorSize = ROOT::Experimental::Internal::RDaosNTupleAnchor::GetSize();
daos_obj_id_t oidMetadata{kOidLowMetadata, static_cast<decltype(daos_obj_id_t::hi)>(this->GetIndex())};

buffer = std::make_unique<unsigned char[]>(anchorSize);
buffer = MakeUninitArray<unsigned char>(anchorSize);
if ((err = cont.ReadSingleAkey(buffer.get(), anchorSize, oidMetadata, kDistributionKeyDefault,
kAttributeKeyAnchor, kCidMetadata))) {
return err;
Expand All @@ -175,8 +176,8 @@ struct RDaosContainerNTupleLocator {
}

builder.SetOnDiskHeaderSize(anchor.fNBytesHeader);
buffer = std::make_unique<unsigned char[]>(anchor.fLenHeader);
zipBuffer = std::make_unique<unsigned char[]>(anchor.fNBytesHeader);
buffer = MakeUninitArray<unsigned char>(anchor.fLenHeader);
zipBuffer = MakeUninitArray<unsigned char>(anchor.fNBytesHeader);
if ((err = cont.ReadSingleAkey(zipBuffer.get(), anchor.fNBytesHeader, oidMetadata, kDistributionKeyDefault,
kAttributeKeyHeader, kCidMetadata)))
return err;
Expand All @@ -185,8 +186,8 @@ struct RDaosContainerNTupleLocator {
ROOT::Experimental::Internal::RNTupleSerializer::DeserializeHeader(buffer.get(), anchor.fLenHeader, builder);

builder.AddToOnDiskFooterSize(anchor.fNBytesFooter);
buffer = std::make_unique<unsigned char[]>(anchor.fLenFooter);
zipBuffer = std::make_unique<unsigned char[]>(anchor.fNBytesFooter);
buffer = MakeUninitArray<unsigned char>(anchor.fLenFooter);
zipBuffer = MakeUninitArray<unsigned char>(anchor.fNBytesFooter);
if ((err = cont.ReadSingleAkey(zipBuffer.get(), anchor.fNBytesFooter, oidMetadata, kDistributionKeyDefault,
kAttributeKeyFooter, kCidMetadata)))
return err;
Expand Down Expand Up @@ -310,7 +311,7 @@ void ROOT::Experimental::Internal::RPageSinkDaos::InitImpl(unsigned char *serial
auto [locator, _] = RDaosContainerNTupleLocator::LocateNTuple(*fDaosContainer, fNTupleName);
fNTupleIndex = locator.GetIndex();

auto zipBuffer = std::make_unique<unsigned char[]>(length);
auto zipBuffer = MakeUninitArray<unsigned char>(length);
auto szZipHeader = fCompressor->Zip(serializedHeader, length, GetWriteOptions().GetCompression(),
RNTupleCompressor::MakeMemCopyWriter(zipBuffer.get()));
WriteNTupleHeader(zipBuffer.get(), szZipHeader, length);
Expand Down Expand Up @@ -431,7 +432,7 @@ ROOT::Experimental::RNTupleLocator
ROOT::Experimental::Internal::RPageSinkDaos::CommitClusterGroupImpl(unsigned char *serializedPageList,
std::uint32_t length)
{
auto bufPageListZip = std::make_unique<unsigned char[]>(length);
auto bufPageListZip = MakeUninitArray<unsigned char>(length);
auto szPageListZip = fCompressor->Zip(serializedPageList, length, GetWriteOptions().GetCompression(),
RNTupleCompressor::MakeMemCopyWriter(bufPageListZip.get()));

Expand All @@ -451,7 +452,7 @@ ROOT::Experimental::Internal::RPageSinkDaos::CommitClusterGroupImpl(unsigned cha
void ROOT::Experimental::Internal::RPageSinkDaos::CommitDatasetImpl(unsigned char *serializedFooter,
std::uint32_t length)
{
auto bufFooterZip = std::make_unique<unsigned char[]>(length);
auto bufFooterZip = MakeUninitArray<unsigned char>(length);
auto szFooterZip = fCompressor->Zip(serializedFooter, length, GetWriteOptions().GetCompression(),
RNTupleCompressor::MakeMemCopyWriter(bufFooterZip.get()));
WriteNTupleFooter(bufFooterZip.get(), szFooterZip, length);
Expand Down Expand Up @@ -479,7 +480,7 @@ void ROOT::Experimental::Internal::RPageSinkDaos::WriteNTupleFooter(const void *
void ROOT::Experimental::Internal::RPageSinkDaos::WriteNTupleAnchor()
{
const auto ntplSize = RDaosNTupleAnchor::GetSize();
auto buffer = std::make_unique<unsigned char[]>(ntplSize);
auto buffer = MakeUninitArray<unsigned char>(ntplSize);
fNTupleAnchor.Serialize(buffer.get());
fDaosContainer->WriteSingleAkey(
buffer.get(), ntplSize, daos_obj_id_t{kOidLowMetadata, static_cast<decltype(daos_obj_id_t::hi)>(fNTupleIndex)},
Expand Down Expand Up @@ -524,8 +525,8 @@ ROOT::Experimental::RNTupleDescriptor ROOT::Experimental::Internal::RPageSourceD
auto desc = descBuilder.MoveDescriptor();

for (const auto &cgDesc : desc.GetClusterGroupIterable()) {
buffer = std::make_unique<unsigned char[]>(cgDesc.GetPageListLength());
zipBuffer = std::make_unique<unsigned char[]>(cgDesc.GetPageListLocator().fBytesOnStorage);
buffer = MakeUninitArray<unsigned char>(cgDesc.GetPageListLength());
zipBuffer = MakeUninitArray<unsigned char>(cgDesc.GetPageListLocator().fBytesOnStorage);
fDaosContainer->ReadSingleAkey(
zipBuffer.get(), cgDesc.GetPageListLocator().fBytesOnStorage, oidPageList, kDistributionKeyDefault,
cgDesc.GetPageListLocator().GetPosition<RNTupleLocatorObject64>().fLocation, kCidMetadata);
Expand Down Expand Up @@ -574,7 +575,7 @@ void ROOT::Experimental::Internal::RPageSourceDaos::LoadSealedPage(DescriptorId_
const auto [position, offset] = DecodeDaosPagePosition(pageInfo.fLocator.GetPosition<RNTupleLocatorObject64>());
RDaosKey daosKey = GetPageDaosKey<kDefaultDaosMapping>(fNTupleIndex, clusterId, physicalColumnId, position);
const auto bufSize = offset + sealedPage.GetBufferSize();
auto cageHeadBuffer = std::make_unique<unsigned char[]>(bufSize);
auto cageHeadBuffer = MakeUninitArray<unsigned char>(bufSize);
fDaosContainer->ReadSingleAkey(cageHeadBuffer.get(), bufSize, daosKey.fOid, daosKey.fDkey, daosKey.fAkey);
memcpy(const_cast<void *>(sealedPage.GetBuffer()), cageHeadBuffer.get() + offset, sealedPage.GetBufferSize());
} else {
Expand Down Expand Up @@ -621,7 +622,7 @@ ROOT::Experimental::Internal::RPageSourceDaos::LoadPageImpl(ColumnHandle_t colum
R__FAIL("accessing caged pages is only supported in conjunction with cluster cache"));
}

directReadBuffer = std::unique_ptr<unsigned char[]>(new unsigned char[sealedPage.GetBufferSize()]);
directReadBuffer = MakeUninitArray<unsigned char>(sealedPage.GetBufferSize());
RDaosKey daosKey = GetPageDaosKey<kDefaultDaosMapping>(
fNTupleIndex, clusterId, columnId, pageInfo.fLocator.GetPosition<RNTupleLocatorObject64>().fLocation);
fDaosContainer->ReadSingleAkey(directReadBuffer.get(), sealedPage.GetBufferSize(), daosKey.fOid, daosKey.fDkey,
Expand Down
10 changes: 5 additions & 5 deletions tree/ntuple/v7/src/RPageStorageFile.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ ROOT::Experimental::Internal::RPageSinkFile::~RPageSinkFile() {}

void ROOT::Experimental::Internal::RPageSinkFile::InitImpl(unsigned char *serializedHeader, std::uint32_t length)
{
auto zipBuffer = std::make_unique<unsigned char[]>(length);
auto zipBuffer = MakeUninitArray<unsigned char>(length);
auto szZipHeader = fCompressor->Zip(serializedHeader, length, GetWriteOptions().GetCompression(),
RNTupleCompressor::MakeMemCopyWriter(zipBuffer.get()));
fWriter->WriteNTupleHeader(zipBuffer.get(), szZipHeader, length);
Expand Down Expand Up @@ -227,7 +227,7 @@ ROOT::Experimental::RNTupleLocator
ROOT::Experimental::Internal::RPageSinkFile::CommitClusterGroupImpl(unsigned char *serializedPageList,
std::uint32_t length)
{
auto bufPageListZip = std::make_unique<unsigned char[]>(length);
auto bufPageListZip = MakeUninitArray<unsigned char>(length);
auto szPageListZip = fCompressor->Zip(serializedPageList, length, GetWriteOptions().GetCompression(),
RNTupleCompressor::MakeMemCopyWriter(bufPageListZip.get()));

Expand All @@ -241,7 +241,7 @@ void ROOT::Experimental::Internal::RPageSinkFile::CommitDatasetImpl(unsigned cha
std::uint32_t length)
{
fWriter->UpdateStreamerInfos(fDescriptorBuilder.BuildStreamerInfos());
auto bufFooterZip = std::make_unique<unsigned char[]>(length);
auto bufFooterZip = MakeUninitArray<unsigned char>(length);
auto szFooterZip = fCompressor->Zip(serializedFooter, length, GetWriteOptions().GetCompression(),
RNTupleCompressor::MakeMemCopyWriter(bufFooterZip.get()));
fWriter->WriteNTupleFooter(bufFooterZip.get(), szFooterZip, length);
Expand Down Expand Up @@ -324,7 +324,7 @@ void ROOT::Experimental::Internal::RPageSourceFile::LoadStructureImpl()
// Reserve enough space for the compressed and the uncompressed header/footer (see AttachImpl)
const auto bufSize = fAnchor->GetNBytesHeader() + fAnchor->GetNBytesFooter() +
std::max(fAnchor->GetLenHeader(), fAnchor->GetLenFooter());
fStructureBuffer.fBuffer = std::make_unique<unsigned char[]>(bufSize);
fStructureBuffer.fBuffer = MakeUninitArray<unsigned char>(bufSize);
fStructureBuffer.fPtrHeader = fStructureBuffer.fBuffer.get();
fStructureBuffer.fPtrFooter = fStructureBuffer.fBuffer.get() + fAnchor->GetNBytesHeader();

Expand Down Expand Up @@ -439,7 +439,7 @@ ROOT::Experimental::Internal::RPageSourceFile::LoadPageImpl(ColumnHandle_t colum
std::unique_ptr<unsigned char[]> directReadBuffer; // only used if cluster pool is turned off

if (fOptions.GetClusterCache() == RNTupleReadOptions::EClusterCache::kOff) {
directReadBuffer = std::unique_ptr<unsigned char[]>(new unsigned char[sealedPage.GetBufferSize()]);
directReadBuffer = MakeUninitArray<unsigned char>(sealedPage.GetBufferSize());
{
Detail::RNTupleAtomicTimer timer(fCounters->fTimeWallRead, fCounters->fTimeCpuRead);
fReader.ReadBuffer(directReadBuffer.get(), sealedPage.GetBufferSize(),
Expand Down

0 comments on commit 1c94d18

Please sign in to comment.