[VecOps] RVec 2.0: small buffer optimization based on LLVM SmallVector #7502
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Starting build on |
DeepCode's analysis on #77efbb found:
Top issues
👉 View analysis in DeepCode’s Dashboard | Configure the bot👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin. |
|
Build failed on ROOT-fedora31/noimt. Failing tests:
|
|
Build failed on ROOT-fedora30/cxx14. Failing tests:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests:
|
|
Build failed on mac1014/python3. Failing tests:
|
|
Starting build on |
|
Build failed on ROOT-fedora31/noimt. Failing tests:
|
|
Build failed on ROOT-ubuntu16/nortcxxmod. Failing tests:
|
|
Build failed on mac11.0/cxx17. Errors:
|
|
Build failed on ROOT-fedora30/cxx14. Failing tests:
|
|
Build failed on ROOT-debian10-i386/cxx14. |
|
Build failed on windows10/cxx14. Errors:
|
|
This is a reproducer for the Python failures: import ROOT
rvec = ROOT.VecOps.RVec("float")()Stacktrace at the point of crash is:
and |
|
I've built with clang10 and debug info and rebased on top of master. Then I get following additional info: [wunsch@portal1 build_native]$ python -c 'import ROOT; rvec = ROOT.VecOps.RVec("float")()'
input_line_14:1:64: error: redefinition of 'is_equal'
namespace __cppyy_internal { template<class C1, class C2> bool is_equal(const C1& c1, const C2& c2) { return (bool)(c1 == c2); } }
^
input_line_10:1:64: note: previous definition is here
namespace __cppyy_internal { template<class C1, class C2> bool is_equal(const C1& c1, const C2& c2) { return (bool)(c1 == c2); } }
^
input_line_15:1:64: error: redefinition of 'is_not_equal'
namespace __cppyy_internal { template<class C1, class C2> bool is_not_equal(const C1& c1, const C2& c2) { return (bool)(c1 != c2); } }
^
input_line_11:1:64: note: previous definition is here
namespace __cppyy_internal { template<class C1, class C2> bool is_not_equal(const C1& c1, const C2& c2) { return (bool)(c1 != c2); } }
^
*** Break *** segmentation violation
===========================================================
There was a crash (kSigSegmentationViolation).
This is the entire stack trace of all threads:
===========================================================
#0 0x00007f0ac351e46c in waitpid () from /lib64/libc.so.6
#1 0x00007f0ac349bf62 in do_system () from /lib64/libc.so.6
#2 0x00007f0abbefaf19 in TUnixSystem::Exec (this=0x193ebd0, shellcmd=0x5886a50 "/work/wunsch/root_rvec/build_native/etc/gdb-backtrace.sh 3925717 1>&2") at /work/wunsch/root_rvec/core/unix/src/TUnixSystem.cxx:2120
#3 0x00007f0abbefba11 in TUnixSystem::StackTrace (this=0x193ebd0) at /work/wunsch/root_rvec/core/unix/src/TUnixSystem.cxx:2411
#4 0x00007f0aa6eb2e91 in (anonymous namespace)::TExceptionHandlerImp::HandleException(int) () from /cvmfs/sft-nightlies.cern.ch/lcg/nightlies/dev3/Mon/ROOT/HEAD/x86_64-centos7-clang10-opt/lib/libcppyy_backend3_8.so
#5 0x00007f0abbf01176 in TUnixSystem::DispatchSignals (this=0x193ebd0, sig=kSigSegmentationViolation) at /work/wunsch/root_rvec/core/unix/src/TUnixSystem.cxx:3644
#6 0x00007f0abbef5f81 in SigHandler (sig=kSigSegmentationViolation) at /work/wunsch/root_rvec/core/unix/src/TUnixSystem.cxx:407
#7 0x00007f0abbf01364 in sighandler (sig=11) at /work/wunsch/root_rvec/core/unix/src/TUnixSystem.cxx:3620
#8 <signal handler called>
#9 0x00007f0abc71ec1c in __gnu_cxx::new_allocator<CPyCppyy::PyCallable*>::construct<CPyCppyy::PyCallable*, CPyCppyy::PyCallable* const&> (this=0x7f0abc784120, __p=0x5f746f6f722f6863, __args=
0x7ffeaf819970: 0x58665b0) at /cvmfs/sft.cern.ch/lcg/releases/gcc/9.2.0-afc57/x86_64-centos7/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/ext/new_allocator.h:147
#10 0x00007f0abc71ea1d in std::allocator_traits<std::allocator<CPyCppyy::PyCallable*> >::construct<CPyCppyy::PyCallable*, CPyCppyy::PyCallable* const&> (__a=..., __p=0x5f746f6f722f6863, __args=
0x7ffeaf819970: 0x58665b0) at /cvmfs/sft.cern.ch/lcg/releases/gcc/9.2.0-afc57/x86_64-centos7/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/alloc_traits.h:484
#11 0x00007f0abc71e81e in std::vector<CPyCppyy::PyCallable*, std::allocator<CPyCppyy::PyCallable*> >::push_back (this=0x7f0abc784120, __x=
0x7ffeaf819970: 0x58665b0) at /cvmfs/sft.cern.ch/lcg/releases/gcc/9.2.0-afc57/x86_64-centos7/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/stl_vector.h:1189
#12 0x00007f0abc72709f in CPyCppyy::CPPOverload::AdoptMethod (this=0x7f0abc9caed8 <CPyCppyy::CPPOverload_Type>, pc=0x58665b0) at /work/wunsch/root_rvec/bindings/pyroot/cppyy/CPyCppyy/src/CPPOverload.cxx:945
#13 0x00007f0abc76e7f0 in CPyCppyy::TemplateProxy::AdoptMethod (this=0x7f0aaca317c0, pc=0x58665b0) at /work/wunsch/root_rvec/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx:111
#14 0x00007f0abc75ed8c in CPyCppyy::BuildScopeProxyDict (scope=14, pyclass=0x5859310) at /work/wunsch/root_rvec/bindings/pyroot/cppyy/CPyCppyy/src/ProxyWrappers.cxx:254
#15 0x00007f0abc75d3de in CPyCppyy::CreateScopeProxy (name=..., parent=0x57021d0) at /work/wunsch/root_rvec/bindings/pyroot/cppyy/CPyCppyy/src/ProxyWrappers.cxx:676
#16 0x00007f0abc7378d8 in (anonymous namespace)::MakeCppTemplateClass (args=0x7f0abca483c0) at /work/wunsch/root_rvec/bindings/pyroot/cppyy/CPyCppyy/src/CPyCppyyModule.cxx:368
#17 0x00007f0ac440cd1a in cfunction_call_varargs (func=0x7f0abca8aef0, args=0x7f0abca483c0, kwargs=<optimized out>) at /workspace/build/externals/Python-3.8.6/src/Python/3.8.6/Objects/call.c:757
#18 0x00007f0ac440d834 in PyCFunction_Call (func=0x7ffeaf819970, args=0x5f746f6f722f6863, kwargs=0x7ffeaf819970) at /workspace/build/externals/Python-3.8.6/src/Python/3.8.6/Objects/call.c:772
#19 0x00007f0ac44d7b0c in do_call_core (tstate=0x1884330, func=0x7f0abca8aef0, callargs=0x7f0abca483c0, kwdict=0x0) at /workspace/build/externals/Python-3.8.6/src/Python/3.8.6/Python/ceval.c:4983 |
|
|
|
With this patch, you can find out which method causes the issue: --- a/bindings/pyroot/cppyy/CPyCppyy/src/CPPOverload.cxx
+++ b/bindings/pyroot/cppyy/CPyCppyy/src/CPPOverload.cxx
@@ -20,6 +20,7 @@
#include <algorithm>
#include <sstream>
#include <vector>
+#include <iostream>
namespace CPyCppyy {
@@ -941,6 +942,13 @@ void CPyCppyy::CPPOverload::Set(const std::string& name, std::vector<PyCallable*
//----------------------------------------------------------------------------
void CPyCppyy::CPPOverload::AdoptMethod(PyCallable* pc)
{
+ PyObject_Print(pc->GetPrototype(), stdout, 0);
+ std::cout << std::endl;
+
+ PyObject_Print(pc->GetSignature(), stdout, 0);
+ std::cout << std::endl;
+
+ std::cout << std::endl;
// Fill in the data of a freshly created method proxy.
fMethodInfo->fMethods.push_back(pc);
fMethodInfo->fFlags &= ~CallContext::kIsSorted;It gets you to the point that you have multiple method with the same signature: ...
'float& ROOT::VecOps::RVec<float>::operator[](unsigned long idx)'
'(unsigned long idx)'
'float& ROOT::VecOps::RVec<float>::operator[](unsigned long idx)'
'(unsigned long idx)' |
|
I don't understand, the problem is not the "redefinition of |
|
My best guess is that cppyy sees a method two times and then defines the equal operator in the jit twice. The iteration over the methods happens here (the counter is there to set a breakpoint more easily and continue until the actual critical part happens): diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/ProxyWrappers.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/ProxyWrappers.cxx
index 23e7ac8..70b662c 100644
--- a/bindings/pyroot/cppyy/CPyCppyy/src/ProxyWrappers.cxx
+++ b/bindings/pyroot/cppyy/CPyCppyy/src/ProxyWrappers.cxx
@@ -26,6 +26,7 @@
#include <set>
#include <string>
#include <vector>
+#include <iostream>
//- data _______________________________________________________________________
@@ -141,6 +142,8 @@ static inline void sync_templates(
Py_DECREF(pyname);
}
+static auto counter = 0u;
+
static int BuildScopeProxyDict(Cppyy::TCppScope_t scope, PyObject* pyclass)
{
// Collect methods and data for the given scope, and add them to the given python
@@ -169,6 +172,9 @@ static int BuildScopeProxyDict(Cppyy::TCppScope_t scope, PyObject* pyclass)
// process the method based on its name
std::string mtCppName = Cppyy::GetMethodName(method);
+ std::cout << "Counter: " << counter << std::endl;
+ counter++;
+ std::cout << "Method name: " << mtCppName << std::endl;
// special case trackers
bool setupSetItem = false; |
|
More info: The And this is again called in various places in https://github.com/root-project/root/blob/master/bindings/pyroot/cppyy/CPyCppyy/src/CPPInstance.cxx And a I tried to reproduce the doubled function with plain |
stwunsch
reviewed
Mar 23, 2021
math/vecops/inc/ROOT/RVec.hxx
Outdated
| return *this; | ||
| } | ||
|
|
||
| using SmallVectorTemplateCommon<T>::operator[]; |
There was a problem hiding this comment.
This using declaration may be the culprit? We have a rather complicated past in terms of using declarations and jitting ;)
|
The It's some weirdness which includes inheritance with also a templated method in the derived class. Here is the most minimal reproducer I could find: import cppyy
cppyy.cppdef('''
template <typename T>
struct Base {
T data;
T& operator[](int idx) { return data; }
};
template <typename T>
struct Vec : public Base<T> {
T data;
// we have two options to trigger the issue:
// 1. Including this line makes it break
//using Base<T>::operator[];
// 2. Or including this line
// Note that you have to derive Vec from Base to make it break!
T& operator[](int idx) { return data; }
// Note that you have to have the template here!
template <typename V>
T& operator[](T idx) {
return data;
}
};
''')
vec = cppyy.gbl.Vec['float']()
print(vec, vec[0])
vec[0] = 42.0
print(vec, vec[0])And here the stacktrace, which is the same as above: #9 0x00007f09d1f0bc1c in __gnu_cxx::new_allocator<CPyCppyy::PyCallable*>::construct<CPyCppyy::PyCallable*, CPyCppyy::PyCallable* const&> (this=0x7f09d1f71120, __p=0x5f746f6f722f6863, __args=
0x7ffd74585880: 0x5f009c0) at /cvmfs/sft.cern.ch/lcg/releases/gcc/9.2.0-afc57/x86_64-centos7/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/ext/new_allocator.h:147
#10 0x00007f09d1f0ba1d in std::allocator_traits<std::allocator<CPyCppyy::PyCallable*> >::construct<CPyCppyy::PyCallable*, CPyCppyy::PyCallable* const&> (__a=..., __p=0x5f746f6f722f6863, __args=
0x7ffd74585880: 0x5f009c0) at /cvmfs/sft.cern.ch/lcg/releases/gcc/9.2.0-afc57/x86_64-centos7/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/alloc_traits.h:484
#11 0x00007f09d1f0b81e in std::vector<CPyCppyy::PyCallable*, std::allocator<CPyCppyy::PyCallable*> >::push_back (this=0x7f09d1f71120, __x=
0x7ffd74585880: 0x5f009c0) at /cvmfs/sft.cern.ch/lcg/releases/gcc/9.2.0-afc57/x86_64-centos7/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/stl_vector.h:1189
#12 0x00007f09d1f1409f in CPyCppyy::CPPOverload::AdoptMethod (this=0x7f09d21b7ed8 <CPyCppyy::CPPOverload_Type>, pc=0x5f009c0) at /work/wunsch/root_rvec/bindings/pyroot/cppyy/CPyCppyy/src/CPPOverload.cxx:945
#13 0x00007f09d1f5b7f0 in CPyCppyy::TemplateProxy::AdoptMethod (this=0x7f09c568c490, pc=0x5f009c0) at /work/wunsch/root_rvec/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx:111
#14 0x00007f09d1f4bd8c in CPyCppyy::BuildScopeProxyDict (scope=8, pyclass=0x5f31f60) at /work/wunsch/root_rvec/bindings/pyroot/cppyy/CPyCppyy/src/ProxyWrappers.cxx:254
#15 0x00007f09d1f4a3de in CPyCppyy::CreateScopeProxy (name=..., parent=0x7f09d223a590) at /work/wunsch/root_rvec/bindings/pyroot/cppyy/CPyCppyy/src/ProxyWrappers.cxx:676
#16 0x00007f09d1f248d8 in (anonymous namespace)::MakeCppTemplateClass (args=0x7f09d2237380) at /work/wunsch/root_rvec/bindings/pyroot/cppyy/CPyCppyy/src/CPyCppyyModule.cxx:368
#17 0x00007f09d9bf9d1a in cfunction_call_varargs (func=0x7f09d21fe360, args=0x7f09d2237380, kwargs=<optimized out>) at /workspace/build/externals/Python-3.8.6/src/Python/3.8.6/Objects/call.c:757
#18 0x00007f09d9bfa834 in PyCFunction_Call (func=0x7ffd74585880, args=0x5f746f6f722f6863, kwargs=0x7ffd74585880) at /workspace/build/externals/Python-3.8.6/src/Python/3.8.6/Objects/call.c:772
#19 0x00007f09d9cc4b0c in do_call_core (tstate=0x23ba020, func=0x7f09d21fe360, callargs=0x7f09d2237380, kwdict=0x0) at /workspace/build/externals/Python-3.8.6/src/Python/3.8.6/Python/ceval.c:4983
#20 _PyEval_EvalFrameDefault (f=0x7f09da1609f0, throwflag=<optimized out>) at /workspace/build/externals/Python-3.8.6/src/Python/3.8.6/Python/ceval.c:3559
#21 0x00007f09d9cc8863 in PyEval_EvalFrameEx (f=0x7f09da1609f0, throwflag=0) at /workspace/build/externals/Python-3.8.6/src/Python/3.8.6/Python/ceval.c:741 |
The previous implementation of the vectorized operator[] was using emplace_back to add elements to the returned vector. emplace_back is slightly more complex in RVec 2.0 due to the SBO and compilers sometimes do not inline it. Instead, spell out the appropriate logic: for example we know the return vector has sufficient capacity and we can skip the related checks performed at every emplace_back call.
We define vectorized ones for RVecs that are used instead.
sizeof(void*) is 4, not 8 on 32bit machines.
As a simple standalone component, let's try to reduce its dependencies to the minimum.
|
Starting build on |
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Starting build on |
|
Starting build on |
|
Build failed on mac11.0/cxx17. Failing tests: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fCapacity == -1to indicate memory-adoption modeasserts tothrowssizeof(T)*8 > 1024 ? 0 : 8or similar, see also https://lists.llvm.org/pipermail/llvm-dev/2020-November/146613.html and the way they currently do it in LLVM: https://llvm.org/doxygen/SmallVector_8h_source.html#l01101)math/vecops/ARCHITECTURE.md)