Skip to content

Commit

Permalink
Refactor/any soo (#3564)
Browse files Browse the repository at this point in the history 
* refactor(Any): SOO

- encapsulate data holders
- add missing gets and ops
- eliminate g++ warnings with enable_if's
- default enable SOO

* refactor(Placeholder): encapsulate SOO memory management and fix leaks; cf. #3297 #3514

* fix(Placeholder): asan errors and add tests

cf. #3297 #3514
  • Loading branch information
@aleks-f
aleks-f committed Apr 16, 2022
1 parent 7ae6b60 commit 9c976da
Show file tree
Hide file tree
Showing 9 changed files with 264 additions and 119 deletions.
150 changes: 99 additions & 51 deletions Foundation/include/Poco/Any.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ template <class T> class VarHolderImpl;
}


template <class T, std::size_t S>
struct TypeSizeLE:
std::integral_constant<bool, (sizeof(T) <= S)>{};


template <class T, std::size_t S>
struct TypeSizeGT:
std::integral_constant<bool, (sizeof(T) > S)>{};


#ifndef POCO_NO_SOO


Expand All @@ -58,14 +68,38 @@ union Placeholder
static const unsigned int value = SizeV;
};

Placeholder()
Placeholder(const Placeholder&) = delete;
Placeholder(Placeholder&&) = delete;
Placeholder& operator=(const Placeholder&) = delete;
Placeholder& operator=(Placeholder&&) = delete;

Placeholder(): pHolder(0)
{
erase();
std::memset(holder, 0, sizeof(Placeholder));
}

~Placeholder()
{
destruct(false);
}

void swap(Placeholder& other)
{
if (!isLocal() && !other.isLocal())
std::swap(pHolder, other.pHolder);
else
throw Poco::InvalidAccessException("Placeholder::swap()");
}

void erase()
{
std::memset(holder, 0, sizeof(Placeholder));
destruct(true);
}

bool isEmpty() const
{
char buf[POCO_SMALL_OBJECT_SIZE] = {};
return 0 == std::memcmp(holder, buf, POCO_SMALL_OBJECT_SIZE);
}

bool isLocal() const
Expand All @@ -78,6 +112,24 @@ union Placeholder
holder[SizeV] = local ? 1 : 0;
}

template <typename T, typename V>
PlaceholderT* assignStack(const V& value)
{
erase();
new (reinterpret_cast<PlaceholderT*>(holder)) T(value);
setLocal(true);
return reinterpret_cast<PlaceholderT*>(holder);
}

template <typename T, typename V>
PlaceholderT* assignHeap(const V& value)
{
erase();
pHolder = new T(value);
setLocal(false);
return pHolder;
}

PlaceholderT* content() const
{
if (isLocal())
Expand All @@ -86,20 +138,25 @@ union Placeholder
return pHolder;
}

// MSVC71,80 won't extend friendship to nested class (Any::Holder)
#if !defined(POCO_MSVC_VERSION) || (defined(POCO_MSVC_VERSION) && (POCO_MSVC_VERSION > 80))
private:
#endif
typedef typename std::aligned_storage<SizeV + 1>::type AlignerType;
typedef typename std::aligned_storage<SizeV+1>::type AlignerType;

PlaceholderT* pHolder;
mutable char holder[SizeV + 1];
AlignerType aligner;
void destruct(bool clear)
{
if (!isEmpty())
{
if (!isLocal())
delete pHolder;
else
reinterpret_cast<PlaceholderT*>(holder)->~PlaceholderT();

friend class Any;
friend class Dynamic::Var;
friend class Dynamic::VarHolder;
template <class> friend class Dynamic::VarHolderImpl;
if (clear) std::memset(holder, 0, sizeof(Placeholder));
}
}

PlaceholderT* pHolder;
mutable unsigned char holder[SizeV+1];
AlignerType aligner;
};


Expand Down Expand Up @@ -185,13 +242,6 @@ class Any
/// Destructor. If Any is locally held, calls ValueHolder destructor;
/// otherwise, deletes the placeholder from the heap.
{
if (!empty())
{
if (_valueHolder.isLocal())
destruct();
else
delete content();
}
}

Any& swap(Any& other)
Expand All @@ -205,14 +255,13 @@ class Any

if (!_valueHolder.isLocal() && !other._valueHolder.isLocal())
{
std::swap(_valueHolder.pHolder, other._valueHolder.pHolder);
_valueHolder.swap(other._valueHolder);
}
else
{
Any tmp(*this);
try
{
if (_valueHolder.isLocal()) destruct();
construct(other);
other = tmp;
}
Expand Down Expand Up @@ -252,8 +301,7 @@ class Any
bool empty() const
/// Returns true if the Any is empty.
{
char buf[POCO_SMALL_OBJECT_SIZE] = { 0 };
return 0 == std::memcmp(_valueHolder.holder, buf, POCO_SMALL_OBJECT_SIZE);
return _valueHolder.isEmpty();
}

const std::type_info & type() const
Expand Down Expand Up @@ -290,21 +338,27 @@ class Any

virtual void clone(Placeholder<ValueHolder>* pPlaceholder) const
{
if ((sizeof(Holder<ValueType>) <= POCO_SMALL_OBJECT_SIZE))
{
new ((ValueHolder*) pPlaceholder->holder) Holder(_held);
pPlaceholder->setLocal(true);
}
else
{
pPlaceholder->pHolder = new Holder(_held);
pPlaceholder->setLocal(false);
}
cloneSOO(pPlaceholder, _held);
}

ValueType _held;

private:

template<typename VT,
typename std::enable_if<TypeSizeLE<Holder<VT>, Placeholder<VT>::Size::value>::value>::type* = nullptr>
static void cloneSOO(Placeholder<ValueHolder>* pPlaceholder, VT& held)
{
pPlaceholder->assignStack<Holder<ValueType>, ValueType>(held);
}

template<typename VT,
typename std::enable_if<TypeSizeGT<Holder<VT>, Placeholder<VT>::Size::value>::value>::type* = nullptr>
static void cloneSOO(Placeholder<ValueHolder>* pPlaceholder, VT& held)
{
pPlaceholder->assignHeap<Holder<ValueType>, ValueType>(held);
}

Holder & operator = (const Holder &);
};

Expand All @@ -313,19 +367,18 @@ class Any
return _valueHolder.content();
}

template<typename ValueType>
template<typename ValueType,
typename std::enable_if<TypeSizeLE<Holder<ValueType>, Placeholder<ValueType>::Size::value>::value>::type* = nullptr>
void construct(const ValueType& value)
{
if (sizeof(Holder<ValueType>) <= Placeholder<ValueType>::Size::value)
{
new (reinterpret_cast<ValueHolder*>(_valueHolder.holder)) Holder<ValueType>(value);
_valueHolder.setLocal(true);
}
else
{
_valueHolder.pHolder = new Holder<ValueType>(value);
_valueHolder.setLocal(false);
}
_valueHolder.assignStack<Holder<ValueType>, ValueType>(value);
}

template<typename ValueType,
typename std::enable_if<TypeSizeGT<Holder<ValueType>, Placeholder<ValueType>::Size::value>::value>::type* = nullptr>
void construct(const ValueType& value)
{
_valueHolder.assignHeap<Holder<ValueType>, ValueType>(value);
}

void construct(const Any& other)
Expand All @@ -336,11 +389,6 @@ class Any
_valueHolder.erase();
}

void destruct()
{
content()->~ValueHolder();
}

Placeholder<ValueHolder> _valueHolder;


Expand Down
8 changes: 2 additions & 6 deletions Foundation/include/Poco/Config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,15 @@
// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
// !!! NOTE: Any/Dynamic::Var SOO will NOT work reliably !!!
// !!! without C++11 (std::aligned_storage in particular). !!!
// !!! Only comment this out if your compiler has support !!!
// !!! for std::aligned_storage. !!!
// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
//
#ifndef POCO_ENABLE_SOO
#define POCO_NO_SOO
#endif
//#define POCO_NO_SOO


// Small object size in bytes. When assigned to Any or Var,
// objects larger than this value will be alocated on the heap,
// while those smaller will be placement new-ed into an
// internal buffer.
// internal stack-auto-allocated buffer.
#if !defined(POCO_SMALL_OBJECT_SIZE) && !defined(POCO_NO_SOO)
#define POCO_SMALL_OBJECT_SIZE 32
#endif
Expand Down
61 changes: 24 additions & 37 deletions Foundation/include/Poco/Dynamic/Var.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -621,10 +621,10 @@ class Foundation_API Var

void destruct()
{
if (!isEmpty()) delete content();
delete _pHolder;
}

VarHolder* _pHolder;
VarHolder* _pHolder = nullptr;

#else

Expand All @@ -633,53 +633,41 @@ class Foundation_API Var
return _placeholder.content();
}

void destruct()
{
}

template<typename ValueType,
typename std::enable_if<TypeSizeLE<VarHolderImpl<ValueType>, Placeholder<ValueType>::Size::value>::value>::type* = nullptr>
void constructSOO(const ValueType& value)
{
_placeholder.assignStack<VarHolderImpl<ValueType>, ValueType>(value);
}

template<typename ValueType,
typename std::enable_if<TypeSizeGT<VarHolderImpl<ValueType>, Placeholder<ValueType>::Size::value>::value>::type* = nullptr>
void constructSOO(const ValueType& value)
{
_placeholder.assignHeap<VarHolderImpl<ValueType>, ValueType>(value);
}

template<typename ValueType>
void construct(const ValueType& value)
{
if (sizeof(VarHolderImpl<ValueType>) <= Placeholder<ValueType>::Size::value)
{
new (reinterpret_cast<VarHolder*>(_placeholder.holder)) VarHolderImpl<ValueType>(value);
_placeholder.setLocal(true);
}
else
{
_placeholder.pHolder = new VarHolderImpl<ValueType>(value);
_placeholder.setLocal(false);
}
constructSOO(value);
}

void construct(const char* value)
{
std::string val(value);
if (sizeof(VarHolderImpl<std::string>) <= Placeholder<std::string>::Size::value)
{
new (reinterpret_cast<VarHolder*>(_placeholder.holder)) VarHolderImpl<std::string>(val);
_placeholder.setLocal(true);
}
else
{
_placeholder.pHolder = new VarHolderImpl<std::string>(val);
_placeholder.setLocal(false);
}
constructSOO(val);
}

void construct(const Var& other)
{
_placeholder.erase();
if (!other.isEmpty())
other.content()->clone(&_placeholder);
else
_placeholder.erase();
}

void destruct()
{
if (!isEmpty())
{
if (_placeholder.isLocal())
content()->~VarHolder();
else
delete content();
}
}

Placeholder<VarHolder> _placeholder;
Expand Down Expand Up @@ -709,14 +697,13 @@ inline void Var::swap(Var& other)

if (!_placeholder.isLocal() && !other._placeholder.isLocal())
{
std::swap(_placeholder.pHolder, other._placeholder.pHolder);
_placeholder.swap(other._placeholder);
}
else
{
Var tmp(*this);
try
{
if (_placeholder.isLocal()) destruct();
construct(other);
other = tmp;
}
Expand Down
Loading

0 comments on commit 9c976da

Please sign in to comment.