Ext3h commented on code in PR #48192:
URL: https://github.com/apache/arrow/pull/48192#discussion_r2569095045
##########
cpp/src/arrow/util/compression_zstd.cc:
##########
@@ -187,9 +202,14 @@ class ZSTDCodec : public Codec {
DCHECK_EQ(output_buffer_len, 0);
output_buffer = &empty_buffer;
}
-
- size_t ret = ZSTD_decompress(output_buffer,
static_cast<size_t>(output_buffer_len),
- input, static_cast<size_t>(input_len));
+ // Decompression context for ZSTD contains several large heap allocations.
Review Comment:
You mean like:
```c++
#include <memory>
#include <mutex>
#include <map>
#include <set>
class LocalContext
{
};
// Container, only updated in rare case. Doubles as factory for
`LocalContext`.
class ContextFactory {
public:
// To be called by `LocalContextReference`.
// Returned context is to be explicitly released if external reference
expires first.
std::shared_ptr<LocalContext> Create() {
std::scoped_lock lock(door_);
return *instances_.emplace(std::make_shared<LocalContext>()).first;
}
void Release(const std::shared_ptr<LocalContext>& instance)
{
assert(instance);
// This will result in LocalContext expiring outside of the mutex to
avoid unnecessary contention.
std::scoped_lock lock(door_);
instances_.erase(instance);
}
private:
// Strong references are held here, but expiry of TLS can clear them out.
std::set<std::shared_ptr<LocalContext>> instances_;
std::mutex door_;
};
// Thread-local cache at the usage location.
class LocalContextReference {
public:
~LocalContextReference() {
// Need to inform all `ContextFactory ` that are still alive, that
they may release the strong reference to `LocalContext` associated with this
thread.
for(const auto& instance : instances_)
{
// Factory can be expired before this.
if(auto factory = instance.first.lock())
{
factory->Release(instance.second.lock());
}
}
}
std::shared_ptr<LocalContext> Get(const std::shared_ptr<ContextFactory>&
factory)
{
assert(factory);
// Not possible until C++26...
// Need std::weak_ptr<T>::owner_equal from C++26.
auto it = instances_.find(factory);
if(it == instances_.end()) {
it = instances_.emplace(std::piecewise_construct,
std::forward_as_tuple(factory), std::forward_as_tuple(factory->Create())).first;
}
return it->second.lock();
}
private:
std::map<std::weak_ptr<ContextFactory>, std::weak_ptr<LocalContext>>
instances_;
};
// At usage location.
thread_local LocalContextReference reference;
```
At ***minimum*** this would require a (reasonable...) backport of
`std::weak_ptr<T>::owner_equal` to work with older C++ standards in the line
`instances_.find(factory)`. We can not use `std::unordered_map` with
`std::weak_ptr` because `std::weak_ptr<T>::owner_hash` - unlike `owner_equal` -
is ***not*** possible to back port as it requires private interfaces on
`std::weak_ptr`.
Apart from that it should behave acceptable. `std::map` in uncontested cache
lines / TLS is surprisingly fast.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
