Skip to content

Commit

Permalink
Replace BitMapObjectPool with configurable ObjectPool (#14459)
Browse files Browse the repository at this point in the history
* Replace BitMapObjectPool with configurable ObjectPool

#### Problem

`ObjectPool` can use either in-line or heap allocation with a
platform-configurable default, but several cases still use the
original inline-only `BitMapObjectPool`.

#### Change overview

- Declare pools as `ObjectPool` rather than `BitMapObjectPool`
  where possible. (Issues for exceptions: #14444, #14446)

- Remove some empty-on-shutdown checks since the pool normally does this
  itself (inhibited for asan+heap testing):
    - ExchangeManager::Shutdown()
    - ~EndPointManagerImplPool()

- Fix many tests to support heap allocation.

#### Testing

CI; no change to visible functionality expected.

* Leave some pools inline

- `ExchangeManager::mContextPool` and
  `ReliableMessageMgr::mContextPool`: issue #14509
- `GroupSessionTable::mEntries`: issue #14512

* Add chip::Test::PlatformMemoryUser

- Add PlatformMemoryUser to manage MemoryInit()/MemoryShutdown()
  for test contexts that may use heap.
- Revert related test changes

* restyle
  • Loading branch information
kpschoedel authored and pull[bot] committed Feb 11, 2022
1 parent a6281fe commit 1689280
Show file tree
Hide file tree
Showing 17 changed files with 65 additions and 29 deletions.
2 changes: 1 addition & 1 deletion src/app/CASEClientPool.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class CASEClientPool : public CASEClientPoolDelegate
void Release(CASEClient * client) override { mClientPool.ReleaseObject(client); }

private:
BitMapObjectPool<CASEClient, N> mClientPool;
ObjectPool<CASEClient, N> mClientPool;
};

}; // namespace chip
6 changes: 3 additions & 3 deletions src/app/InteractionModelEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -283,12 +283,12 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman

CommandHandlerInterface * mCommandHandlerList = nullptr;

BitMapObjectPool<CommandHandler, CHIP_IM_MAX_NUM_COMMAND_HANDLER> mCommandHandlerObjs;
BitMapObjectPool<TimedHandler, CHIP_IM_MAX_NUM_TIMED_HANDLER> mTimedHandlers;
ObjectPool<CommandHandler, CHIP_IM_MAX_NUM_COMMAND_HANDLER> mCommandHandlerObjs;
ObjectPool<TimedHandler, CHIP_IM_MAX_NUM_TIMED_HANDLER> mTimedHandlers;
ObjectPool<ReadHandler, CHIP_IM_MAX_NUM_READ_HANDLER> mReadHandlers;
WriteHandler mWriteHandlers[CHIP_IM_MAX_NUM_WRITE_HANDLER];
reporting::Engine mReportingEngine;
BitMapObjectPool<ClusterInfo, CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS> mClusterInfoPool;
ObjectPool<ClusterInfo, CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS> mClusterInfoPool;

ReadClient * mpActiveReadClientList = nullptr;

Expand Down
2 changes: 1 addition & 1 deletion src/app/OperationalDeviceProxyPool.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class OperationalDeviceProxyPool : public OperationalDeviceProxyPoolDelegate
}

private:
BitMapObjectPool<OperationalDeviceProxy, N> mDevicePool;
ObjectPool<OperationalDeviceProxy, N> mDevicePool;
};

}; // namespace chip
2 changes: 1 addition & 1 deletion src/app/clusters/bindings/BindingManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ class BindingManager
}

private:
BitMapObjectPool<PendingNotificationEntry, EMBER_BINDING_TABLE_SIZE> mPendingNotificationMap;
ObjectPool<PendingNotificationEntry, EMBER_BINDING_TABLE_SIZE> mPendingNotificationMap;
};

static void HandleDeviceConnected(void * context, OperationalDeviceProxy * device);
Expand Down
2 changes: 1 addition & 1 deletion src/app/reporting/Engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ class Engine
* mGlobalDirtySet is used to track the set of attribute/event paths marked dirty for reporting purposes.
*
*/
BitMapObjectPool<ClusterInfo, CHIP_IM_SERVER_MAX_NUM_DIRTY_SET> mGlobalDirtySet;
ObjectPool<ClusterInfo, CHIP_IM_SERVER_MAX_NUM_DIRTY_SET> mGlobalDirtySet;

#if CONFIG_IM_BUILD_FOR_UNIT_TEST
uint32_t mReservedSize = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
CommissioningStage mCommissioningStage = CommissioningStage::kSecurePairing;
bool mRunCommissioningAfterConnection = false;

BitMapObjectPool<CommissioneeDeviceProxy, kNumMaxActiveDevices> mCommissioneeDevicePool;
ObjectPool<CommissioneeDeviceProxy, kNumMaxActiveDevices> mCommissioneeDevicePool;

#if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY // make this commissioner discoverable
UserDirectedCommissioningServer * mUdcServer = nullptr;
Expand Down
12 changes: 6 additions & 6 deletions src/credentials/GroupDataProviderImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -217,12 +217,12 @@ class GroupDataProviderImpl : public GroupDataProvider

chip::PersistentStorageDelegate & mStorage;
bool mInitialized = false;
BitMapObjectPool<GroupInfoIteratorImpl, kIteratorsMax> mGroupInfoIterators;
BitMapObjectPool<GroupKeyIteratorImpl, kIteratorsMax> mGroupKeyIterators;
BitMapObjectPool<EndpointIteratorImpl, kIteratorsMax> mEndpointIterators;
BitMapObjectPool<KeySetIteratorImpl, kIteratorsMax> mKeySetIterators;
BitMapObjectPool<GroupSessionIteratorImpl, kIteratorsMax> mGroupSessionsIterator;
BitMapObjectPool<GroupKeyContext, kIteratorsMax> mKeyContexPool;
ObjectPool<GroupInfoIteratorImpl, kIteratorsMax> mGroupInfoIterators;
ObjectPool<GroupKeyIteratorImpl, kIteratorsMax> mGroupKeyIterators;
ObjectPool<EndpointIteratorImpl, kIteratorsMax> mEndpointIterators;
ObjectPool<KeySetIteratorImpl, kIteratorsMax> mKeySetIterators;
ObjectPool<GroupSessionIteratorImpl, kIteratorsMax> mGroupSessionsIterator;
ObjectPool<GroupKeyContext, kIteratorsMax> mKeyContexPool;
};

} // namespace Credentials
Expand Down
2 changes: 1 addition & 1 deletion src/credentials/tests/TestGroupDataProvider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1200,12 +1200,12 @@ int Test_Setup(void * inContext)
*/
int Test_Teardown(void * inContext)
{
chip::Platform::MemoryShutdown();
GroupDataProvider * provider = GetGroupDataProvider();
if (nullptr != provider)
{
provider->Finish();
}
chip::Platform::MemoryShutdown();
return SUCCESS;
}

Expand Down
4 changes: 2 additions & 2 deletions src/inet/InetLayer.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ class EndPointManagerImplPool : public EndPointManager<typename EndPointImpl::En
using Manager = EndPointManager<typename EndPointImpl::EndPoint>;
using EndPoint = typename EndPointImpl::EndPoint;

EndPointManagerImplPool() = default;
~EndPointManagerImplPool() { VerifyOrDie(sEndPointPool.Allocated() == 0); }
EndPointManagerImplPool() = default;
~EndPointManagerImplPool() = default;

EndPoint * CreateEndPoint() override { return sEndPointPool.CreateObject(*this); }
void ReleaseEndPoint(EndPoint * endPoint) override { sEndPointPool.ReleaseObject(static_cast<EndPointImpl *>(endPoint)); }
Expand Down
11 changes: 10 additions & 1 deletion src/lib/support/Pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,16 @@ class HeapObjectPool : public internal::Statistics, public internal::PoolCommon<
{
public:
HeapObjectPool() {}
~HeapObjectPool() { VerifyOrDie(Allocated() == 0); }
~HeapObjectPool()
{
#if __SANITIZE_ADDRESS__
// Free all remaining objects so that ASAN can catch specific use-after-free cases.
ReleaseAll();
#else // __SANITIZE_ADDRESS__
// Verify that no live objects remain, to prevent potential use-after-free.
VerifyOrDie(Allocated() == 0);
#endif // __SANITIZE_ADDRESS__
}

template <typename... Args>
T * CreateObject(Args &&... args)
Expand Down
6 changes: 0 additions & 6 deletions src/messaging/ExchangeMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,6 @@ CHIP_ERROR ExchangeManager::Shutdown()
{
mReliableMessageMgr.Shutdown();

mContextPool.ForEachActiveObject([](auto * ec) {
// There should be no active object in the pool
VerifyOrDie(false);
return Loop::Continue;
});

if (mSessionManager != nullptr)
{
mSessionManager->SetMessageDelegate(nullptr);
Expand Down
1 change: 1 addition & 0 deletions src/messaging/tests/MessagingContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ CHIP_ERROR MessagingContext::Init(TransportMgrBase * transport, IOContext * ioCo
mIOContext = ioContext;
mTransport = transport;

ReturnErrorOnFailure(PlatformMemoryUser::Init());
ReturnErrorOnFailure(mSessionManager.Init(&GetSystemLayer(), transport, &mMessageCounterManager));

ReturnErrorOnFailure(mExchangeManager.Init(&mSessionManager));
Expand Down
33 changes: 32 additions & 1 deletion src/messaging/tests/MessagingContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,42 @@
namespace chip {
namespace Test {

/**
* @brief
* Test contexts that use Platform::Memory and might call Free() on destruction can inherit from this class and call its Init().
* Platform::MemoryShutdown() will then be called after the subclasses' destructor.
*/
class PlatformMemoryUser
{
public:
PlatformMemoryUser() : mInitialized(false) {}
~PlatformMemoryUser()
{
if (mInitialized)
{
chip::Platform::MemoryShutdown();
}
}
CHIP_ERROR Init()
{
CHIP_ERROR status = CHIP_NO_ERROR;
if (!mInitialized)
{
status = chip::Platform::MemoryInit();
mInitialized = (status == CHIP_NO_ERROR);
}
return status;
}

private:
bool mInitialized;
};

/**
* @brief The context of test cases for messaging layer. It wil initialize network layer and system layer, and create
* two secure sessions, connected with each other. Exchanges can be created for each secure session.
*/
class MessagingContext
class MessagingContext : public PlatformMemoryUser
{
public:
MessagingContext() :
Expand Down
2 changes: 1 addition & 1 deletion src/protocols/secure_channel/CASESessionCache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ CHIP_ERROR CASESessionCache::Add(CASESessionCachable & cachableSession)
VerifyOrReturnError(mCachePool.Capacity() > 0, CHIP_NO_ERROR);

// If the cache is full, get the least recently used session index and release that.
if (mCachePool.Exhausted())
if (mCachePool.Allocated() >= kCacheSize)
{
mCachePool.ReleaseObject(GetLRUSession());
}
Expand Down
3 changes: 2 additions & 1 deletion src/protocols/secure_channel/CASESessionCache.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ class CASESessionCache
CHIP_ERROR Get(const PeerId & peer, CASESessionCachable & outCachableSession);

private:
BitMapObjectPool<CASESessionCachable, CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE> mCachePool;
static constexpr size_t kCacheSize = CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE;
ObjectPool<CASESessionCachable, kCacheSize> mCachePool;
CASESessionCachable * GetLRUSession();
};

Expand Down
2 changes: 1 addition & 1 deletion src/transport/SessionManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate

SessionMessageDelegate * mCB = nullptr;

BitMapObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES>
ObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES>
mSessionRecoveryDelegates;

TransportMgrBase * mTransportMgr = nullptr;
Expand Down
2 changes: 1 addition & 1 deletion src/transport/UnauthenticatedSessionTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ class UnauthenticatedSessionTable
return false;
}

BitMapObjectPool<UnauthenticatedSession, kMaxSessionCount> mEntries;
ObjectPool<UnauthenticatedSession, kMaxSessionCount> mEntries;
};

} // namespace Transport
Expand Down

0 comments on commit 1689280

Please sign in to comment.