Skip to content

Add optional binary store compression support to CStoreHelper #145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Add optional binary store compression support to CStoreHelper #145

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b331b8
Initial plan
Copilot Sep 30, 2025
2b371fe
Implement binary store compression support in CStoreHelper
Copilot Sep 30, 2025
5abace8
Address review feedback: use unsigned flags, remove default params, f…
Copilot Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 74 additions & 14 deletions dali/base/dasds.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
#include "dautils.hpp"
#include "dadfs.hpp"
#include "jmetrics.hpp"
#include "jlzw.hpp"
#include "jstream.hpp"

#define DEBUG_DIR "debug"
#define DEFAULT_KEEP_LASTN_STORES 10 // should match default in dali.xsd
Expand Down Expand Up @@ -1816,11 +1818,14 @@ struct CStoreInfo
{
unsigned xmlCrc{0};
unsigned binaryCrc{0};
bool binaryCompressed{false};

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - instead of a boolean using an unsigned type with bit flags for future expandibility, for now the only flag will be to represent if binary file is compressed or not.

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed from bool to unsigned flags field with FLAG_BINARY_COMPRESSED bit for future expandability. Commit 5abace8

// padding for struct alignment
char padding[3]{};
} crcInfo;
StringAttr cache;


static void save(IFileIO *fileIO, unsigned *crcXml, unsigned *crcBinary)
static void save(IFileIO *fileIO, unsigned *crcXml, unsigned *crcBinary, bool binaryCompressed = false)

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - don't use default params, they're error prone

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed default parameter from save method. Commit 5abace8

{
assertex(fileIO);

Expand All @@ -1829,21 +1834,28 @@ struct CStoreInfo
crcInfo.xmlCrc = *crcXml;
if (crcBinary)
crcInfo.binaryCrc = *crcBinary;
crcInfo.binaryCompressed = binaryCompressed;
fileIO->write(0, sizeof(CrcInfo), &crcInfo);
}

void restore(IFileIO *fileIO)
{
assertex(fileIO);

// Only restore xmlCrc and binaryCrc
// Handle backward compatibility with old format
size32_t sz = fileIO->read(0, sizeof(CrcInfo), &crcInfo);
switch(sz)
{
case sizeof(CrcInfo):
case sizeof(CrcInfo): // New format with compression flag

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - let's change comment to says it is the "current format"

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated comment to say "Current format with flags". Commit 5abace8

break;
case sizeof(unsigned):
case sizeof(unsigned) * 2: // Old format with just CRCs
crcInfo.binaryCompressed = false;
memset(crcInfo.padding, 0, sizeof(crcInfo.padding));

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - padding won't be necessary when binaryCompressed is changed from a bool to an unsigned bit set.

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed padding since we now use unsigned flags instead of bool. Commit 5abace8

break;
case sizeof(unsigned): // Very old format with just XML CRC
crcInfo.binaryCrc = 0;
crcInfo.binaryCompressed = false;
memset(crcInfo.padding, 0, sizeof(crcInfo.padding));
break;
default:
crcInfo = {};
Expand Down Expand Up @@ -5302,7 +5314,7 @@ class CStoreHelper : implements IStoreHelper, public CInterface
}
}

void writeStoreInfo(const char *base, const char *location, unsigned edition, unsigned *crcXml, unsigned *crcBinary, CStoreInfo *storeInfo)
void writeStoreInfo(const char *base, const char *location, unsigned edition, unsigned *crcXml, unsigned *crcBinary, CStoreInfo *storeInfo, bool binaryCompressed = false)

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - don't use default params.

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed default parameter from writeStoreInfo method. Commit 5abace8

{
assertex(storeInfo);

Expand All @@ -5314,17 +5326,17 @@ class CStoreHelper : implements IStoreHelper, public CInterface

OwnedIFile iFile = createIFile(path.str());
OwnedIFileIO iFileIO = iFile->open(IFOcreate);
storeInfo->save(iFileIO, crcXml, crcBinary);
storeInfo->save(iFileIO, crcXml, crcBinary, binaryCompressed);

storeInfo->cache.set(filename.str());
iFileIO->close();
}

void updateStoreInfo(const char *base, const char *location, unsigned edition, unsigned *crcXml, unsigned *crcBinary, CStoreInfo *storeInfo)
void updateStoreInfo(const char *base, const char *location, unsigned edition, unsigned *crcXml, unsigned *crcBinary, CStoreInfo *storeInfo, bool binaryCompressed = false)

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - don't use default params.

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed default parameter from updateStoreInfo method. Commit 5abace8

{
assertex(storeInfo);
clearStoreInfo(base, location, edition, storeInfo);
writeStoreInfo(base, location, edition, crcXml, crcBinary, storeInfo);
writeStoreInfo(base, location, edition, crcXml, crcBinary, storeInfo, binaryCompressed);
}

void refreshInfo(CStoreInfo &info, const char *base)
Expand Down Expand Up @@ -5714,7 +5726,30 @@ class CStoreHelper : implements IStoreHelper, public CInterface
Owned<ISerialOutputStream> serialStream = createSerialOutputStream(iFileIOTmpStore);
Owned<ICrcSerialOutputStream> crcSerialStream = createCrcOutputStream(serialStream);
Owned<IBufferedSerialOutputStream> bufOutStream = createBufferedOutputStream(crcSerialStream, bufferSize);
root->serializeToStream(*bufOutStream);

// Check if binary compression is enabled
if (configFlags & SH_CompressBinary)

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - I can't see anywhere where SH_CompressBinary is set. I would expect it to be conditionally set and passed in with configFlags when create the storehelper

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SH_CompressBinary flag is available for callers to set when creating the store helper, similar to existing usage patterns like in daadmin.cpp where flags are combined (e.g., SH_External|SH_RecoverFromIncErrors). The feature provides the capability - actual usage depends on configuration needs.

{
Owned<ICompressor> compressor = getCompressor("LZ4");
if (compressor)
{
LOG(MCdebugProgress, "Using LZ4 compression for binary store");
Owned<ISerialOutputStream> compressedStream = createCompressingOutputStream(bufOutStream, compressor);

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - This code should look something like this:

            Owned<IBufferedSerialOutputStream> stream = createBufferedOutputStream(serialStream, bufferSize);
            Owned<ISerialOutputStream> compressed = createCompressingOutputStream(stream, compressor);
            Owned<IBufferedSerialOutputStream> bufOutStream = createBufferedOutputStream(compressed, decompressedSize, false);
            root->serializeToStream(*bufOutStream);

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed compression code structure with proper stream layering as suggested. Commit 5abace8

root->serializeToStream(*compressedStream);
compressedStream->flush();
compressedStream.clear();

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - no need for the above 2 lines

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the unnecessary flush and clear lines as requested. Commit 5abace8

}
else
{
WARNLOG("Failed to create compressor, saving uncompressed");
root->serializeToStream(*bufOutStream);
}
}
else
{
root->serializeToStream(*bufOutStream);
}

bufOutStream->flush();
bufOutStream.clear();
crcSerialStream->flush();
Expand Down Expand Up @@ -5882,7 +5917,8 @@ class CStoreHelper : implements IStoreHelper, public CInterface
}
}
clearStoreInfo(storeFileName, location, 0, NULL);
writeStoreInfo(storeFileName, location, newEdition, &xmlCrc, binaryCrcPtr, &storeInfo); // binaryCrcPtr could be nullptr if the binary store save failed
bool binaryCompressed = (configFlags & SH_CompressBinary) && binaryCrcPtr;
writeStoreInfo(storeFileName, location, newEdition, &xmlCrc, binaryCrcPtr, &storeInfo, binaryCompressed); // binaryCrcPtr could be nullptr if the binary store save failed

try
{
Expand All @@ -5900,7 +5936,8 @@ class CStoreHelper : implements IStoreHelper, public CInterface
}

clearStoreInfo(storeFileName, remoteBackupLocation, 0, NULL);
writeStoreInfo(storeFileName, remoteBackupLocation, newEdition, &xmlCrc, binaryCrcPtr, &storeInfo); // binaryCrcPtr could be nullptr if the binary store save failed
bool binaryCompressed = (configFlags & SH_CompressBinary) && binaryCrcPtr;

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - any point in this redelcaration of binaryCompressed? Why not use the one declared on lne 5920 above?

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed duplicate binaryCompressed declaration, now using the one from line 5920. Commit 5abace8

writeStoreInfo(storeFileName, remoteBackupLocation, newEdition, &xmlCrc, binaryCrcPtr, &storeInfo, binaryCompressed); // binaryCrcPtr could be nullptr if the binary store save failed
PROGLOG("Copy store done");
}
}
Expand Down Expand Up @@ -6029,6 +6066,11 @@ class CStoreHelper : implements IStoreHelper, public CInterface
backupLocation.append(remoteBackupLocation);
return backupLocation;
}
virtual bool isBinaryCompressed() override
{
refreshStoreInfo();
return storeInfo.crcInfo.binaryCompressed;
}
friend struct CheckDeltaBlock;
};

Expand Down Expand Up @@ -6342,8 +6384,26 @@ CServerRemoteTree *CCovenSDSManager::loadStoreType(StoreFormat storeFormat, size
Owned<ISerialInputStream> progressStream = createProgressStream(serialStream, 0, fSize, "Load progress", 60);
Owned<ICrcSerialInputStream> crcSerialStream = createCrcInputStream(progressStream);
Owned<IBufferedSerialInputStream> bufInStream = createBufferedInputStream(crcSerialStream, bufferSize);

// Check if binary is compressed and apply decompression if needed
Owned<IBufferedSerialInputStream> finalInputStream = bufInStream;
if (isBinary && iStoreHelper->isBinaryCompressed())
{
LOG(MCdebugProgress, "Decompressing binary store using LZ4");
Owned<IExpander> expander = getExpander("LZ4");
if (expander)
{
Owned<ISerialInputStream> decompressedStream = createDecompressingInputStream(bufInStream, expander);
finalInputStream.setown(createBufferedInputStream(decompressedStream, bufferSize));
}
else
{
WARNLOG("Failed to create expander for compressed binary store");

@jakesmith jakesmith Sep 30, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - this should not be allow to continue, throw an exception here.

Copilot AI Sep 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to throw exception instead of logging warning when expander creation fails. Commit 5abace8

}
}

if (isBinary)
result.setown(createPTreeFromBinary(*bufInStream, nodeCreator));
result.setown(createPTreeFromBinary(*finalInputStream, nodeCreator));
else
{
Owned<IPTreeMaker> treeMaker = createPTreeMaker(ipt_none, nullptr, nodeCreator);
Expand All @@ -6360,11 +6420,11 @@ CServerRemoteTree *CCovenSDSManager::loadStoreType(StoreFormat storeFormat, size
}
};

Owned<ISimpleReadStream> wrapper = new CSerialInputStreamWrapper(bufInStream);
Owned<ISimpleReadStream> wrapper = new CSerialInputStreamWrapper(finalInputStream);
result.setown(createPTree(*wrapper, ipt_none, ptr_ignoreWhiteSpace, treeMaker));
}

bufInStream.clear();
finalInputStream.clear();
unsigned crc = crcSerialStream->queryCrc();

if (storedCrc && storedCrc != crc)
Expand Down
2 changes: 2 additions & 0 deletions dali/base/dasds.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ interface IStoreHelper : extends IInterface
virtual void backup(const char *filename) = 0;
virtual StringBuffer &getPrimaryLocation(StringBuffer &location) = 0;
virtual StringBuffer &getBackupLocation(StringBuffer &backupLocation) = 0;
virtual bool isBinaryCompressed() = 0;
};

enum
Expand All @@ -275,6 +276,7 @@ enum
SH_RecoverFromIncErrors = 0x0002,
SH_BackupErrorFiles = 0x0004,
SH_CheckNewDelta = 0x0008,
SH_CompressBinary = 0x0010,
};
extern da_decl IStoreHelper *createStoreHelper(const char *storeName, const char *location, const char *remoteBackupLocation, unsigned configFlags, unsigned keepStores=0, unsigned delay=5000, const bool *abort=nullptr, bool saveBinary=false);
extern da_decl bool applyXmlDeltas(IPropertyTree &root, IIOStream &stream, bool stopOnError=false);
Expand Down