Skip to content

A few changes to properly pass storeResidual to BLAST #16

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

Closed
genevb wants to merge 4 commits into from
Closed

A few changes to properly pass storeResidual to BLAST #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f490dd
Must add 2 before bit-shifting, as 255+2 will affect second byte, etc.
genevb Aug 13, 2022
64be44e
Ensure properly passing storeResidual to BLAST
genevb Aug 14, 2022
80c1b18
Remove useless line
genevb Aug 14, 2022
5dc6997
cxlevels array should not be modified past its last element
genevb Aug 14, 2022
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
4 changes: 2 additions & 2 deletions core/accelogic/inc/ZipAccelogic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

#include "EDataType.h"

void R__zipBLAST(int *cxlevels, int *srcsize, char *src, int *tgtsize, char **tgts, int tgt_number, int *irep, EDataType datatype = EDataType::kNoType_t);
void R__zipBLAST(int *cxlevels, int *srcsize, char *src, int *tgtsize, char **tgts, int tgt_number, int *irep, bool storeResidual, EDataType datatype = EDataType::kNoType_t);

void R__unzipBLAST(int *srcsize, unsigned char **srcs, int *tgtsize, unsigned char *tgt, int src_number, int *irep);

Expand All @@ -24,7 +24,7 @@ void R__unzipBLAST(int *srcsize, unsigned char **srcs, int *tgtsize, unsigned ch
extern "C" {
#endif

void R__zipBLAST(int cxlevel, int *srcsize, char *src, int *tgtsize, char *tgt, int *irep, EDataType datatype = EDataType::kNoType_t);
void R__zipBLAST(int cxlevel, int *srcsize, char *src, int *tgtsize, char *tgt, int *irep, bool storeResidual, EDataType datatype = EDataType::kNoType_t);

void R__unzipBLAST(int *srcsize, unsigned char *src, int *tgtsize, unsigned char *tgt, int *irep);

Expand Down
28 changes: 14 additions & 14 deletions core/accelogic/src/ZipAccelogic.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@ union IntegerTypes {
ULong64_t *ull;
};

void R__zipBLAST(int cxlevel, int *srcsize, char *src, int *tgtsize, char *tgt, int *irep, EDataType datatype)
void R__zipBLAST(int cxlevel, int *srcsize, char *src, int *tgtsize, char *tgt, int *irep, bool storeResidual, EDataType datatype)
{
R__zipBLAST(&cxlevel,srcsize,src,tgtsize,&tgt,1,irep,datatype);
R__zipBLAST(&cxlevel,srcsize,src,tgtsize,&tgt,1,irep,storeResidual,datatype);
}

void R__zipBLAST(int *cxlevels, int *srcsize, char *src, int *tgtsize, char **tgts, int tgt_number, int *irep, EDataType datatype)
void R__zipBLAST(int *cxlevels, int *srcsize, char *src, int *tgtsize, char **tgts, int tgt_number, int *irep, bool storeResidual, EDataType datatype)
{
memset(irep,0,tgt_number*sizeof(int)); // irep needs to point to an array of integers of size tgt_number (could just be a single integer)
char *tgt = *tgts;
Expand Down Expand Up @@ -87,24 +87,23 @@ void R__zipBLAST(int *cxlevels, int *srcsize, char *src, int *tgtsize, char **tg
size_t float_number = *srcsize / elsize;
// Use "absSens".
int absSensLevels[MAX_ZIG_BUFFERS];
// blast1_compress needs to know whether to keep the residual, and does not count
// the residual among the target buffers. Final buffer's cxlevel
// is irrelevant if it will be the residual buffer.
auto absSens_tgt_number = tgt_number - (storeResidual ? 1 : 0);
// We shift the request config from [1,71] to [-60, 10]
for (int tgt_idx=0; tgt_idx<tgt_number; tgt_idx++)
for (int tgt_idx=0; tgt_idx<absSens_tgt_number; tgt_idx++)
absSensLevels[tgt_idx] = cxlevels[tgt_idx] - 61;
// blast1_compress needs to know whether to keep the residual, and does not count
// the residual among the target buffers. We use cxlevel=0 for final buffer to
// indicate whether it will be the residual buffer.
auto needresidual = (cxlevels[tgt_number-1] == 0);
auto absSens_tgt_number = tgt_number - (needresidual ? 1 : 0);
// Note: We need to check the source really start of a float boundary.
// Note: We need to upgrade blast to avoid the memcpy (which is IN ADDITION to an internal copy already!!!)
char *staging[MAX_ZIG_BUFFERS] = { nullptr };
RealTypes source;
source.c = src;

if (isfloat)
blast1_compress<true>(absSensLevels, source.f, float_number, staging, out_sizes, absSens_tgt_number, needresidual);
blast1_compress<true>(absSensLevels, source.f, float_number, staging, out_sizes, absSens_tgt_number, storeResidual);
else
blast1_compress<true>(absSensLevels, source.d, float_number, staging, out_sizes, absSens_tgt_number, needresidual);
blast1_compress<true>(absSensLevels, source.d, float_number, staging, out_sizes, absSens_tgt_number, storeResidual);

auto excessive_size = false;
for (int tgt_idx=0; tgt_idx<tgt_number && !excessive_size; tgt_idx++)
Expand Down Expand Up @@ -188,9 +187,10 @@ void R__zipBLAST(int *cxlevels, int *srcsize, char *src, int *tgtsize, char **tg
// tgt[2] is set for each target buffer above

// Include the 2 extra header byte into the out size.
tgt[3] = (char)((out_sizes[tgt_idx] + 2) & 0xff);
tgt[4] = (char)((out_sizes[tgt_idx] >> 8) & 0xff);
tgt[5] = (char)((out_sizes[tgt_idx] >> 16) & 0xff);
size_t out_size = out_sizes[tgt_idx] + 2;
Copy link
Copy Markdown
Owner

@pcanal pcanal Aug 15, 2022

Choose a reason for hiding this comment

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

Is this change cosmetic?

Copy link
Copy Markdown
Author

@genevb genevb Aug 15, 2022

Choose a reason for hiding this comment

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

No. As you had written it, if out_sizes[tgt_idx] = 255, then tgt[3] = 1, tgt[4] = 0, and tgt[5] = 0, which would be interpreted as having a size of 1. The way I wrote it, tgt[4] would also become 1, so that the true size of 255 + 2 = 257 is preserved.

Copy link
Copy Markdown
Owner

@pcanal pcanal Aug 15, 2022

Choose a reason for hiding this comment

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

Right. Thanks!

tgt[3] = (char)(out_size & 0xff);
tgt[4] = (char)((out_size >> 8) & 0xff);
tgt[5] = (char)((out_size >> 16) & 0xff);

tgt[6] = (char)(in_size & 0xff); /* decompressed size */
tgt[7] = (char)((in_size >> 8) & 0xff);
Expand Down
15 changes: 7 additions & 8 deletions core/zip/src/RZip.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,24 +130,23 @@ void R__zipPrecisionCascade(int *srcsize, char *src, int *tgtsize, char **tgts,
memset(irep,0,tgt_number*sizeof(int));
return;
}

auto content = reinterpret_cast<ROOT::Internal::PrecisionCascadeConfigArrayContent*>(configarray);
(void) configsize;
Copy link
Copy Markdown
Owner

@pcanal pcanal Aug 15, 2022

Choose a reason for hiding this comment

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

Since the assert is the only use of configsize and it is removed in Release build, this line prevent the compiler from complaining about unused variable/parameter.

assert(content && (content->SizeOf() == (size_t)configsize));
Int_t *cxlevels = content->GetLevels(); // This an array of size `content->fLen`

for (int tgt_idx=0; tgt_idx<tgt_number; tgt_idx++) {
// can only be 0 for the last of multiple (not just one) targets
// otherwise must be a positive value
int cxlevel_min = (tgt_idx > 0 && tgt_idx == tgt_number-1 ? 0 : 1);
if (cxlevels[tgt_idx] < cxlevel_min) {
// do not accept negative levels, and only accept level 0 if it is the final level,
// as this is a proxy for storing the residual of a cascade
bool storeResidual = content->fStoreResidual || (tgt_number > 0 && cxlevels[tgt_number-1] == 0);
int tgt_checks = tgt_number - (storeResidual ? 1 : 0);
Copy link
Copy Markdown
Owner

@pcanal pcanal Aug 14, 2022

Choose a reason for hiding this comment

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

Humm that seems wrong. If 'content->fStoreResidual', the last element is probably not zero, unless we explicitly already added it (in which case the if can be simplified to tgt_number > 0 && cxlevels[tgt_number-1] == 0), isn't it?

Copy link
Copy Markdown
Author

@genevb genevb Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for double-checking me on this, Philippe.

I have observed that if content->fStoreResidua', then tgt_number, which is passed via precisionCascades.size() from TBasket, is 1 larger than content->fLen, so there is no point in examining cxlevels[tgt_number-1] in that case because the user never set that many levels. In other words, if I as a user provide 5 levels, and turn on fStoreResidual, then precisionCascades will have a size of 6 (it already has an extra buffer for the residual) and we only want to check the value of the 5 user-provided levels [0]...[4], not [5]. So I think this is the correct thing to do.

Copy link
Copy Markdown
Owner

@pcanal pcanal Aug 15, 2022

Choose a reason for hiding this comment

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

but what if the user put both a zero at the end and has fStoreResidual == true ?

Copy link
Copy Markdown
Author

@genevb genevb Aug 15, 2022

Choose a reason for hiding this comment

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

Yeah, right now it is set to fail on that with the check on the level being less than 1. The code you've written will create two buffers and output files for the residual in that case, so if it is to be allowed, it should be caught further upstream of this to avoid creating two buffers/files for the residual.

Copy link
Copy Markdown
Owner

@pcanal pcanal Aug 15, 2022

Choose a reason for hiding this comment

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

Maybe the simplest is to not support '0' as a compression level (and possible explicitly reject it in PrecisionCascadeCompressionConfig::PrecisionCascadeCompressionConfig in Compression.cxx. then the code above can be simplified, right?

Copy link
Copy Markdown
Author

@genevb genevb Aug 15, 2022

Choose a reason for hiding this comment

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

Perhaps this was the only controversial point in this PR?

Copy link
Copy Markdown
Owner

@pcanal pcanal Aug 15, 2022

Choose a reason for hiding this comment

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

yes but since we migrate the i/o to store the 'storeResidual' at the end of the levels array (adding it based on the user input), it seems most of the PR is moot. Let me finish the change and see what I need to import.

for (int tgt_idx=0; tgt_idx<tgt_checks; tgt_idx++) {
if (cxlevels[tgt_idx] < 1) {
memset(irep,0,tgt_number*sizeof(int));
return;
}
}

if (compressionAlgorithm == ROOT::RCompressionSetting::EAlgorithm::kBLAST) {
R__zipBLAST(cxlevels, srcsize, src, tgtsize, tgts, tgt_number, irep, datatype);
R__zipBLAST(cxlevels, srcsize, src, tgtsize, tgts, tgt_number, irep, storeResidual, datatype);
}
}

Expand Down