Skip to content
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

Jump to bottom

PrecisionCascade connections to compression functions #13

Merged
merged 14 commits into from
Jun 19, 2022
Merged

PrecisionCascade connections to compression functions #13

merged 14 commits into from
Jun 19, 2022

Conversation

genevb
Copy link

@genevb genevb commented Jun 17, 2022

Missing: array of compression levels (contents for cxlevels vector)
Not sure I have everything else correct in TBasket, but it does compile for me.
Makes PR #12 obsolete.

@genevb genevb requested a review from pcanal as a code owner June 17, 2022 09:26
pcanal
pcanal reviewed Jun 17, 2022
View reviewed changes
tree/tree/src/TBasket.cxx Outdated Show resolved Hide resolved
pcanal
pcanal reviewed Jun 17, 2022
View reviewed changes
tree/tree/src/TBasket.cxx Outdated Show resolved Hide resolved
pcanal
pcanal reviewed Jun 17, 2022
View reviewed changes
tree/tree/src/TBasket.cxx Outdated
@@ -1284,8 +1301,16 @@ Int_t TBasket::WriteBuffer()
R__SizeBuffer(*cascade_buffer, buflen);
cascade_buffer->SetWriteMode();
precisionCascades.push_back( cascade_buffer->Buffer() + cascade_basket->GetKeylen() );
cxlevels.push_back(cxlevel); // WRONG! From where do we get the cxlevels of the precision cascade?
Copy link
Owner

@pcanal pcanal Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It comes from the configArray and to extract the information (should probably be done within R__zip....)

if (configArray && configArray->IsPrecisionCascade()) { // just in case something is wrong
    // oups I have not uploaded the implementation yet :(
}

Copy link
Author

Choose a reason for hiding this comment

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

OK....I'll await your implementation and then I'll write the relevant code in RZip.cxx.

Copy link
Owner

Choose a reason for hiding this comment

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

See https://github.com/pcanal/root/pull/13/files#r900478838

pcanal
pcanal reviewed Jun 17, 2022
View reviewed changes
tree/tree/src/TBasket.cxx Outdated Show resolved Hide resolved
@genevb genevb mentioned this pull request Jun 17, 2022
Closed
pcanal
pcanal reviewed Jun 17, 2022
View reviewed changes
core/zip/src/RZip.cxx Outdated Show resolved Hide resolved
pcanal
pcanal reviewed Jun 17, 2022
View reviewed changes
core/zip/src/RZip.cxx
memset(irep,0,tgt_number*sizeof(int));
return;
}

Copy link
Owner

@pcanal pcanal Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
auto content = reinterpret_cast<ROOT::Internal::PrecisionCascadeConfigArrayContent*>(fConfigArray);
assert(content->SizeOf() == configsize);
Int_t *cxlevels = content->GetLevels(); // This an array of size `content->fLen`

Copy link
Owner

@pcanal pcanal Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The compression levels are the user provided levels, so they still need offset by 61.

Copy link
Owner

Choose a reason for hiding this comment

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

#include "PrecisionCascadeConfigArrayContent.h" is needed.

pcanal
pcanal reviewed Jun 17, 2022
View reviewed changes
tree/tree/src/TBasket.cxx
// increment each element by its corresponding nout.
}
for (size_t i = 0; i<precisionCascades.size(); i++) // does nothing if no precision cascade
precisionCascades[i] += nouts[i]; // increment each element by its corresponding nout.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
precisionCascades[i] += nouts[i]; // increment each element by its corresponding nout.
precisionCascades[i] += nouts[i + 1]; // increment each element by its corresponding nout.

Copy link
Author

@genevb genevb Jun 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand the suggestion to look at the nouts value from one ahead. The first thing I do with precisionCascades is push_back the primary buffer, so both it and nouts have the primary buffer first and there should be no offset between them.

Copy link
Owner

Choose a reason for hiding this comment

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

yes, I forgot that early push, so you are right.

@genevb
Copy link
Author

genevb commented Jun 18, 2022

These two files are still to be written?
#include "PrecisionCascadeCompressionConfig.h" (from Compression.cxx)
#include "PrecisionCascadeConfigArrayContent.h" (from RZip.cxx)

@pcanal
Copy link
Owner

pcanal commented Jun 18, 2022 •
edited
Loading

I had forgotten to push the file and had misnamed it .... sorry :(

@genevb
Copy link
Author

genevb commented Jun 19, 2022

Looks like compilation is happy now on my end, except that it warns that configsize is unused, though it is actually used inside the assert in RZip.cxx. What else is needed before I begin testing? I'll have to figure out how to use the configuration for PrecisionCascade, but presumably I can try the old applyBlast.C macro modified to use a a PrecisionCascade.

@pcanal
Copy link
Owner

pcanal commented Jun 19, 2022

Since it compiles, I will rebase and merge.

@pcanal
Copy link
Owner

pcanal commented Jun 19, 2022

The usage is "simple":

PrecisionCascadeCompressionConfig config( RCompressionSetting::EAlgorithm::kBLAST, { 51, 61 }, true /* want to keep the residual / all the precision);
mybranch->SetCompressionSettings(config);

genevb and others added 14 commits June 19, 2022 10:29
@genevb @pcanal
ADjustments to return compressed sizes
90279db
@genevb @pcanal
Modify RZip for ZIG in BLAST compression
cc28611
@genevb @pcanal
Pass PrecisionCascade buffers to RZIP for ZIG compression in BLAST
6adb9af
@genevb @pcanal
Send single buffer PrecisionCascade cases to PrecisionCascade (un)zip…
2485a87 
…pers
@genevb @pcanal
Uncoment config arguments
4b28e98 
Co-authored-by: Philippe Canal <[email protected]>
@genevb @pcanal
PrecicionCascade compression levels are retrieved from config array
ae4b52c
@genevb @pcanal
check content pointer before using it
eed001c
@genevb @pcanal
Pass PrecisionCascade buffers to RZIP for ZIG compression in BLAST
a9066c1
@genevb @pcanal
Send single buffer PrecisionCascade cases to PrecisionCascade (un)zip…
9d264a7 
…pers
@genevb @pcanal
PrecicionCascade compression levels are retrieved from config array
b3a0015
@genevb @pcanal
Modify RZip for ZIG in BLAST compression
43a4afa
@genevb @pcanal
Send single buffer PrecisionCascade cases to PrecisionCascade (un)zip…
d0b1426 
…pers
@genevb @pcanal
Precision Cascade compression levels are retrieved from config array
0bbdd65
@genevb @pcanal
Resolve some small compilation complaints
0b5f911
@pcanal
Copy link
Owner

pcanal commented Jun 19, 2022

@genevb I force pushed to your branch, you probably will have an easier time starting from a new branch for further development.

@pcanal pcanal merged commit 468a32f into pcanal:AccelogicMain Jun 19, 2022
@genevb genevb deleted the AccelogicZIG5 branch September 2, 2022 20:58
pcanal pushed a commit that referenced this pull request Sep 26, 2024
@vepadulano
[treeplayer] Fix memory management of C-style array branches in test
5ad0863 
The test was dynamically allocating the array data members of the `Data` struct, but never deallocating them. This commit polishes the `Data` struct definition and ensures proper management of the data members.

The previous way of writing data to the TTree was leading to a bad memory access in the ReadBasicPointer inlined function in TStreamerInfoReadBuffer.cxx while reading the `double*` array. In particular, the issue arises when accessing and then deallocating the array at the current index provided by the `TCompInfo` object.

```
Target 0: (repro.out) stopped.
(lldb)
Process 13498 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step in
    frame #0: 0x00000001044cf140 libRIO.so`int TStreamerInfo::ReadBuffer<char**>(this=<unavailable>, b=<unavailable>, arr=<unavailable>, compinfo=<unavailable>, first=<unavailable>, last=<unavailable>, narr=<unavailable>, eoffset=<unavailable>, arrayMode=0) at TStreamerInfoReadBuffer.cxx:923:65 [opt]
   920 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kLong:   ReadBasicPointer(Long_t);  continue;
   921 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kLong64: ReadBasicPointer(Long64_t);  continue;
   922 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kFloat:  ReadBasicPointer(Float_t);  continue;
-> 923 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kDouble: ReadBasicPointer(Double_t);  continue;
   924 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUChar:  ReadBasicPointer(UChar_t);  continue;
   925 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUShort: ReadBasicPointer(UShort_t);  continue;
   926 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUInt:   ReadBasicPointer(UInt_t);  continue;
Target 0: (repro.out) stopped.
(lldb)
Process 13498 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step in
    frame #0: 0x00000001044cf184 libRIO.so`int TStreamerInfo::ReadBuffer<char**>(TBuffer&, char** const&, TStreamerInfo::TCompInfo* const*, int, int, int, int, int) [inlined] TBuffer::BufferSize(this=0x000060e00010ef00) const at TBuffer.h:98:41 [opt]
   95  	   TObject *GetParent()  const;
   96  	   char    *Buffer()     const { return fBuffer; }
   97  	   char    *GetCurrent() const { return fBufCur; }
-> 98  	   Int_t    BufferSize() const { return fBufSize; }
   99  	   void     DetachBuffer() { fBuffer = nullptr; }
   100 	   Int_t    Length()     const { return (Int_t)(fBufCur - fBuffer); }
   101 	   void     Expand(Int_t newsize, Bool_t copy = kTRUE);  // expand buffer to newsize
Target 0: (repro.out) stopped.
(lldb) p fBufSize
(Int_t) 32008
(lldb) s
Process 13498 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step in
    frame #0: 0x00000001044cf194 libRIO.so`int TStreamerInfo::ReadBuffer<char**>(this=<unavailable>, b=<unavailable>, arr=<unavailable>, compinfo=<unavailable>, first=<unavailable>, last=<unavailable>, narr=<unavailable>, eoffset=<unavailable>, arrayMode=0) at TStreamerInfoReadBuffer.cxx:923:65 [opt]
   920 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kLong:   ReadBasicPointer(Long_t);  continue;
   921 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kLong64: ReadBasicPointer(Long64_t);  continue;
   922 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kFloat:  ReadBasicPointer(Float_t);  continue;
-> 923 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kDouble: ReadBasicPointer(Double_t);  continue;
   924 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUChar:  ReadBasicPointer(UChar_t);  continue;
   925 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUShort: ReadBasicPointer(UShort_t);  continue;
   926 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUInt:   ReadBasicPointer(UInt_t);  continue;
Target 0: (repro.out) stopped.
(lldb) s
Process 13498 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbebebebebebebeae)
    frame #0: 0x0000000107bac674 libclang_rt.asan_osx_dynamic.dylib`__asan::Allocator::Deallocate(void*, unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType) + 76
libclang_rt.asan_osx_dynamic.dylib`__asan::Allocator::Deallocate:
->  0x107bac674 <+76>: casalb w8, w9, [x22]
    0x107bac678 <+80>: cmp    w8, #0x2
    0x107bac67c <+84>: b.ne   0x107bac6f4    ; <+204>
    0x107bac680 <+88>: mov    x8, #-0x100000000 ; =-4294967296
Target 0: (repro.out) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbebebebebebebeae)
  * frame #0: 0x0000000107bac674 libclang_rt.asan_osx_dynamic.dylib`__asan::Allocator::Deallocate(void*, unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType) + 76
    frame #1: 0x0000000107c0c444 libclang_rt.asan_osx_dynamic.dylib`wrap__ZdaPv + 232
    frame #2: 0x00000001044d4a60 libRIO.so`int TStreamerInfo::ReadBuffer<char**>(this=<unavailable>, b=<unavailable>, arr=<unavailable>, compinfo=<unavailable>, first=<unavailable>, last=<unavailable>, narr=<unavailable>, eoffset=<unavailable>, arrayMode=0) at TStreamerInfoReadBuffer.cxx:923:65 [opt]
    frame #3: 0x0000000103ffc888 libRIO.so`TStreamerInfoActions::GenericReadAction(buf=0x000060e00010ef00, addr=0x0000602000056bd0, config=0x0000604000149910) at TStreamerInfoActions.cxx:195:45
    frame #4: 0x0000000103caa5ec libRIO.so`TStreamerInfoActions::TConfiguredAction::operator()(this=0x00006030001693f0, buffer=0x000060e00010ef00, object=0x0000602000056bd0) const at TStreamerInfoActions.h:123:17
    frame #5: 0x0000000103ca9ef8 libRIO.so`TBufferFile::ApplySequence(this=0x000060e00010ef00, sequence=0x000060600011ac20, obj=0x0000602000056bd0) at TBufferFile.cxx:3702:10
    frame #6: 0x00000001064bc570 libTree.so`TBranchElement::ReadLeavesMemberBranchCount(this=0x0000619000566380, b=0x000060e00010ef00) at TBranchElement.cxx:4603:6
    frame #7: 0x0000000106455ce4 libTree.so`TBranch::GetEntry(this=0x0000619000566380, entry=0, getall=0) at TBranch.cxx:1753:4
    frame #8: 0x00000001064a1764 libTree.so`TBranchElement::GetEntry(this=0x0000619000566380, entry=0, getall=0) at TBranchElement.cxx:2783:27
    frame #9: 0x000000010739915c libTreePlayer.so`ROOT::Detail::TBranchProxy::Read(this=0x00006110000c9580) at TBranchProxy.h:163:42
    frame #10: 0x0000000107649ba8 libTreePlayer.so`(anonymous namespace)::TObjectArrayReader::At(this=0x0000603000169900, proxy=0x00006110000c9580, idx=1) at TTreeReaderArray.cxx:176:22
    frame #11: 0x000000010000c2e4 repro.out`ROOT::Internal::TTreeReaderArrayBase::UntypedAt(this=0x000000016fdfe740, idx=1) const at TTreeReaderArray.h:41:62
    frame #12: 0x000000010000c200 repro.out`TTreeReaderArray<double>::At(this=0x000000016fdfe740, idx=1) at TTreeReaderArray.h:205:54
    frame #13: 0x00000001000065e0 repro.out`TTreeReaderArray<double>::operator[](this=0x000000016fdfe740, idx=1) at TTreeReaderArray.h:207:44
    frame #14: 0x0000000100007b48 repro.out`simpleTest() at repro.cpp:123:26
    frame #15: 0x0000000100007e10 repro.out`main at repro.cpp:128:5
    frame #16: 0x000000018c718274 dyld`start + 2840
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcanal pcanal pcanal left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@genevb @pcanal