Repository navigation
[C++] Overhead of std::shared_ptr<DataType> copies is causing thread contention #31567
Description
We created a benchmark to measure ExecuteScalarExpression performance in ARROW-16014. We noticed significant thread contention (even though there shouldn't be much, if any, for this task) As part of ARROW-16138 we have been investigating possible causes.
One cause seems to be contention from copying shared_ptr objects.
Two possible solutions jump to mind and I'm sure there are many more.
ExecBatch is an internal type and used inside of ExecuteScalarExpression as well as inside of the execution engine. In the former we can safely assume the data types will exist for the duration of the call. In the latter we can safely assume the data types will exist for the duration of the execution plan. Thus we can probably take a more targetted fix and migrate only ExecBatch to using DataType* (or const DataType&).
On the other hand, we might consider a more global approach. All of our "stock" data types are assumed to have static storage duration. However, we must use std::shared_ptr because users could create their own extension types. We could invent an "extension type registration" system where extension types must first be registered with the C++ lib before being used. Then we could have long-lived DataType instances and we could replace std::shared_ptr with DataType* (or const DataType&) throughout most of the entire code base.
But, as I mentioned, I'm sure there are many approaches to take. CC @lidavidm and @pitrou and @cyb70289 for thoughts but this might be interesting for just about any C++ dev.
Reporter: Weston Pace / @westonpace
Related issues:
Original Issue Attachments:
Note: This issue was originally created as ARROW-16161. Please see the migration documentation for further details.
Activity
asfimport commentedon Apr 9, 2022
Antoine Pitrou / @pitrou:
Not really, parametric types generally cannot use static instances.
asfimport commentedon Apr 9, 2022
Antoine Pitrou / @pitrou:
I think the right approach would be to introduce a lighter-weight ExecSpan that would not have ownership of the carried data and types. This assumes the execution machinery keeps ownership to those independently.
asfimport commentedon Apr 9, 2022
Antoine Pitrou / @pitrou:
Note that ExecBatch holds shared_ptr to DataTypes indirectly through the Datums as well. So doing this is more involved that it seems, IMHO.
asfimport commentedon Apr 9, 2022
David Li / @lidavidm:
Can we start by making a function to duplicate a DataType (instead of copying the shared_ptr) and see if using that in targeted places helps? Since that shouldn't need to manipulate a shared reference count, and will be a much less involved refactor. Otherwise I agree with Antoine, the extensive use of Datum will also complicate things. We have been talking about redesigning all this for a while, so it might be a worthwhile, if very involved, project for 9.0.0.
asfimport commentedon Apr 12, 2022
Weston Pace / @westonpace:
Yes, this renders my registry suggestion pretty much invalid. I like the idea of duplicating DataType to start with but I don't know if that gets us closer to an eventual solution. For ExecBatch for example we might be able to state that ExecBatch cannot outlive the producing ExecPlan and do references/pointers.
Datum does complicate things. Datums can be created independently of an ExecPlan and are a pretty established part of the public API.
I spoke with @zagto and he is going to try prototyping something to see what kind of effect it has on this benchmark.
asfimport commentedon Apr 12, 2022
Tobias Zagorni / @zagto:
One of the bottlenecks can already be improved by making
Expression::type()return a reference instead of copying the{}shared_ptr{}.Before:
Benchmark Time CPU Iterations UserCounters...
ExecuteScalarExpressionOverhead/simple_expression/rows_per_batch:100/threads:1 84196667 ns 84133137 ns 8 rows_per_second=118.859M/s
ExecuteScalarExpressionOverhead/simple_expression/rows_per_batch:100/threads:16 37440825 ns 596181958 ns 16 rows_per_second=16.7734M/s
After:
Benchmark Time CPU Iterations UserCounters...
ExecuteScalarExpressionOverhead/simple_expression/rows_per_batch:100/threads:1 75623644 ns 75585333 ns 9 rows_per_second=132.301M/s
ExecuteScalarExpressionOverhead/simple_expression/rows_per_batch:100/threads:16 23834056 ns 380139943 ns 16 rows_per_second=26.3061M/s
The rest seem to be involved with Datum, I'll try what @lidavidm suggested tomorrow
asfimport commentedon Apr 14, 2022
Tobias Zagorni / @zagto:
I managed to make all data types required to compile the benchmark copyable (shallow for now), and it looks this can indeed speed things up. The best configuration of when to copy the DataType so far is on constructing any ArrayData
With this I already get between 35-40 M/s
I'm sure this is still is not optimal since I still see atomic operations in shared_ptr take significant amounts of time, but less than before.
I'll try if I can replace all shared_ptr uses with an own class for some more extreme experiments, like copying every single time, or neither copy nor reference count and just leak instances for comparision.
That said I still expect the solution to be to rebuild ExecBatch in a way it doesn't constantly reference count (or copy) in Datum/ValueDescr.
asfimport commentedon Apr 14, 2022
David Li / @lidavidm:
I agree the ideal is to refactor ExecBatch. I suggested copying as just a way to test the impact without having to refactor to start with.
asfimport commentedon Apr 27, 2022
Tobias Zagorni / @zagto:
I created a hacky prototype of an ExecArrayData class that does not hold a shared_ptr to DataType. It is used in Datum as an alternative to ArrayData. I can currently run all the ExecuteScalarExpressionOverhead benchmarks with it.
There are noticeable performance improvements in multi-threaded runs with small (e.g. 1000) batch sizes. The best is in zero_copy_expression:
old: ExecuteScalarExpressionOverhead/zero_copy_expression/rows_per_batch:1000/threads:16 1613271 ns 25730049 ns 32 rows_per_second=388.651M/snew: ExecuteScalarExpressionOverhead/zero_copy_expression/rows_per_batch:1000/threads:16 1121647 ns 17913402 ns 32 rows_per_second=558.241M/s(full results are attached: ExecArrayData-difference.txt)
In other places I see a small performance reduction, and also the simple_expression/100 improvement is smaller than with the first copyable DataType experiment. Probably due to the additional code paths in this implementation and my way of getting a shared_ptr back from ExecArrayData, cloning the DataType object, which is far from optimal. I'm still looking into this.
most of the remaining reference counting overhead seems to come from 3 places:
ValueDescr / resolving output types using it
Scalar datums
pointers to the CPUDevice instance, because CPUMemeoryManager instances are recreated a lot. I'll create a diffrent subtask for this
Code - (ignore the weird mix of template and subclassing for ExecArrayData)
asfimport commentedon May 4, 2022
Tobias Zagorni / @zagto:
In the last few days I experimented with this a bit more and came up with some additional smaller modifications to reduce thread contention:
make
CPUMemoryManagerobjects exist once in the respective pool, and use raw pointers to reference them, instead of re-creatingCPUMemoryManagerinstances all the time and reference them by{}shared_ptr{}. This should be valid in all cases for CPUMemoryManager, since aCPUMemoryManagerhas a raw pointer to it's pool, and using this shared_ptr'dCPUMemoryManagerbeyond the pool's lifetime wouldn't make much sense anyways. Still need to check if this all works on the CUDA side.make
CPUDevice::Instance()return a reference to it'sshared_ptrinstead of copying it (shouldn't matter much once we have the above)make
ExecBatchIterator::Next()not useSlice()on small batches that would only result in one slice. Instead move the inputThe performance improvement of each of these should only be a few percent (at least in the 1000/10000 items batch sizes we use in master). I had trouble benchmarking them individually on top of the master branch, where there is a lot of contention anyways. On my work laptop general variance is just too high, probably due to things like thermal throteling and power budgets. By applying all three of these on top of the
{}ExecBatchIterator{}, I currently get 40-50 M/s compared to ~27 onsimple_expression/rows_per_batch:100/threads:16for example.I think these 3 could be good canidates to create a PR soon once can confirm that they are beneficial on a more reliable benchmarking setup.
I noticed a reason why the
ExecArrayDatamodification alone did not lead to an as big performance improvement. I did still use a regular array as input and started usingExecArrayDatafrom the first operator onwards. It turns out theArrayDatais already copied twice in the setup before the execution (or once, if we stop slicing the input using the third modification)Additionally, at this stage, ValueDescrs are created from the
ArrayDatatype. UsingExecArrayDatahere would likely go hand-in-hand with also modifyingValueDescrto not contain a{}shared_ptr{}.Modifying
ValueDescrto not contain ashared_ptris probably desireable on its own.ValueDescrseems to be responsible for about half theshared_ptr<DataType>copies, mostly inGetDescriptors()andOutputType::Resolve().I played with it a bit today, but couldn't get it to work yet, since, there are a lot of places that use
ValueDescr::typedirectly, of which some need to be modified.asfimport commentedon May 18, 2022
Tobias Zagorni / @zagto:
I created a PR for avoiding calls to Slice() as ARROW-16562
asfimport commentedon Sep 4, 2022
Todd Farmer / @toddfarmer:
This issue was last updated over 90 days ago, which may be an indication it is no longer being actively worked. To better reflect the current state, the issue is being unassigned per project policy. Please feel free to re-take assignment of the issue if it is being actively worked, or if you plan to start that work soon.