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

Caching mechanism to prevent dynamic_cast in RooAbsArg::setProxyNormSet #12

Merged
merged 1 commit into from
Dec 10, 2021
Merged

Caching mechanism to prevent dynamic_cast in RooAbsArg::setProxyNormSet #12

merged 1 commit into from
Dec 10, 2021

Conversation

gartrog
Copy link
Collaborator

@gartrog gartrog commented Dec 10, 2021

No description provided.

hageboeck
hageboeck approved these changes Dec 10, 2021
View reviewed changes
Copy link
Owner

@hageboeck hageboeck left a comment

Choose a reason for hiding this comment

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

I have one suggestion to minimise the size of the class in memory.

roofit/roofitcore/inc/RooAbsArg.h
RooRefArray _proxyList ; // list of proxies
ProxyListCache _proxyListCache; //! cache of the list of proxies
Copy link
Owner

Choose a reason for hiding this comment

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

It would be nice to move this down somehow, since the bool will force the compiler to add padding bytes to have _proxyList aligned.
You can minimise the size of the class by putting this right before another bool.

Copy link
Owner

Choose a reason for hiding this comment

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

e.g. line 672

Copy link
Owner

Choose a reason for hiding this comment

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

... better right before _valueDirty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, I had no clue about such things. Fixed as you suggested

@hageboeck
Copy link
Owner

No big deal. I'll merge and move the struct around on my branch.

@hageboeck hageboeck merged commit 488c947 into RFHackathon Dec 10, 2021
@hageboeck
Copy link
Owner

On second thought:
RooAbsArg is big enough that those few bytes don't matter. It's probably better to keep it in the correct "logical" place.

@gartrog
Copy link
Collaborator Author

gartrog commented Dec 10, 2021

Ah, I had not seen your last comment. Well, there is a separate PR for the fix, merge it or not depending on what you prefer :)

hageboeck pushed a commit that referenced this pull request Jun 3, 2022
@ellert @Axel-Naumann
Avoid crashes due to static initialization order
c5d0601 
Most commonly seen on ppc64le. Backtrace:

===========================================================
The lines below might hint at the cause of the crash.
You may get help by asking at the ROOT forum https://root.cern/forum
Only if you are really convinced it is a bug in ROOT then please submit a
report at https://root.cern/bugs Please post the ENTIRE stack trace
from above as an attachment in addition to anything else
that might help us fixing this issue.
===========================================================
 #11 ROOT::Experimental::RColor::toHex[abi:cxx11](unsigned char) (v=<optimized out>) at /usr/include/c++/11/ext/new_allocator.h:82
 #12 0x00007fff90c220ec in ROOT::Experimental::RColor::SetRGB (this=0x7fffeadf5d10, r=<optimized out>, g=<optimized out>, b=<optimized out>) at /usr/include/c++/11/ext/new_allocator.h:89
hageboeck pushed a commit that referenced this pull request Sep 18, 2024
@ellert @guitargeek
[pyroot] Whitelist libtbbmalloc in library import test
f14867b 
   6/1344 Test   #12: pyunittests-pyroot-import-load-libs .......................................***Failed    1.35 sec
test_import (import_load_libs.ImportLoadLibs.test_import)
Test libraries loaded after importing ROOT ... ERROR
======================================================================
ERROR: test_import (import_load_libs.ImportLoadLibs.test_import)
Test libraries loaded after importing ROOT
----------------------------------------------------------------------
Exception: Found not whitelisted libraries after importing ROOT:
 - libtbbmalloc
If the test fails with a library that is loaded on purpose, please add it to the whitelist.
----------------------------------------------------------------------
Ran 1 test in 1.186s
FAILED (errors=1)

This failure is seen in Fedora 41 after tbb was updated from version
2021.11.0 to version 2021.13.0 in  the distribution.
hageboeck pushed a commit that referenced this pull request Sep 25, 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 root-project#14: 0x0000000100007b48 repro.out`simpleTest() at repro.cpp:123:26
    frame root-project#15: 0x0000000100007e10 repro.out`main at repro.cpp:128:5
    frame root-project#16: 0x000000018c718274 dyld`start + 2840
```
hageboeck pushed a commit that referenced this pull request Oct 3, 2024
@ellert @guitargeek
[pyroot] Whitelist libtbbmalloc in library import test
b4ca2f8 
   6/1344 Test   #12: pyunittests-pyroot-import-load-libs .......................................***Failed    1.35 sec
test_import (import_load_libs.ImportLoadLibs.test_import)
Test libraries loaded after importing ROOT ... ERROR
======================================================================
ERROR: test_import (import_load_libs.ImportLoadLibs.test_import)
Test libraries loaded after importing ROOT
----------------------------------------------------------------------
Exception: Found not whitelisted libraries after importing ROOT:
 - libtbbmalloc
If the test fails with a library that is loaded on purpose, please add it to the whitelist.
----------------------------------------------------------------------
Ran 1 test in 1.186s
FAILED (errors=1)

This failure is seen in Fedora 41 after tbb was updated from version
2021.11.0 to version 2021.13.0 in  the distribution.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hageboeck hageboeck hageboeck approved these changes

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
@gartrog @hageboeck