Skip to content

Commit ea5241d

Browse files
Levi ArmstrongLevi-Armstrong
Levi Armstrong
authored and
Levi-Armstrong
committed
Improve clone cache unit tests and fix issues with getting clone
1 parent aac43c4 commit ea5241d

File tree

3 files changed

+185
-16
lines changed

3 files changed

+185
-16
lines changed

‎tesseract_common/include/tesseract_common/clone_cache.h‎

Lines changed: 61 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
TESSERACT_COMMON_IGNORE_WARNINGS_PUSH
3232
#include <deque>
3333
#include <memory>
34+
#include <algorithm>
3435
#include <mutex>
3536
#include <console_bridge/console.h>
3637
TESSERACT_COMMON_IGNORE_WARNINGS_POP
@@ -81,7 +82,13 @@ class CloneCache
8182
return nullptr;
8283

8384
if (cache_.empty())
84-
return original_->clone();
85+
{
86+
std::shared_ptr<CacheType> cache = getClone();
87+
if (cache == nullptr)
88+
return nullptr;
89+
90+
return cache;
91+
}
8592

8693
// If the cache needs updating, then update it
8794
std::unique_lock<std::mutex> lock(cache_mutex_);
@@ -90,9 +97,17 @@ class CloneCache
9097
// Update if possible
9198
std::shared_ptr<CacheType> t;
9299
if constexpr (has_member_func_signature_update<CacheType>::value)
100+
{
93101
cache_.back()->update(original_);
102+
}
94103
else
95-
cache_.back() = original_->clone();
104+
{
105+
std::shared_ptr<CacheType> cache = getClone();
106+
if (cache == nullptr)
107+
return nullptr;
108+
109+
cache_.back() = cache;
110+
}
96111
t = cache_.back();
97112
cache_.pop_back();
98113
return t;
@@ -118,11 +133,21 @@ class CloneCache
118133
}
119134

120135
/**
121-
* @brief Get the cache size
122-
* @return The size of the cache.
136+
* @brief Get the set cache size
137+
* @return The set size of the cache.
123138
*/
124139
long getCacheSize() const { return static_cast<long>(cache_size_); }
125140

141+
/**
142+
* @brief Get the current size of the cache
143+
* @return The current size fo the cache
144+
*/
145+
long getCurrentCacheSize() const
146+
{
147+
std::unique_lock<std::mutex> lock(cache_mutex_);
148+
return static_cast<long>(cache_.size());
149+
}
150+
126151
/** @brief If original_ has changed it will update or rebuild the cache of objects */
127152
void updateCache()
128153
{
@@ -139,24 +164,28 @@ class CloneCache
139164
{
140165
// Update if possible
141166
if constexpr (has_member_func_signature_update<CacheType>::value)
167+
{
142168
cache->update(original_);
169+
}
143170
else
144-
cache = original_->clone();
171+
{ // Update is not available to assign new clone
172+
cache = getClone();
173+
}
145174
}
146175
}
147176

177+
// Prune nullptr
178+
cache_.erase(std::remove_if(cache_.begin(),
179+
cache_.end(),
180+
[](const std::shared_ptr<CacheType>& cache) { return (cache == nullptr); }),
181+
cache_.end());
182+
148183
while (cache_.size() < cache_size_)
149184
{
150185
CONSOLE_BRIDGE_logDebug("Adding clone to the cache. Current cache size: %i", cache_.size());
151-
try
152-
{
153-
cache_.push_back(original_->clone());
154-
}
155-
catch (std::exception& e)
156-
{
157-
CONSOLE_BRIDGE_logError("Clone Cache failed to update cache with the following exception: %s", e.what());
158-
break;
159-
}
186+
std::shared_ptr<CacheType> clone = getClone();
187+
if (clone != nullptr)
188+
cache_.push_back(clone);
160189
}
161190
}
162191

@@ -165,17 +194,33 @@ class CloneCache
165194
protected:
166195
void createClone()
167196
{
168-
std::shared_ptr<CacheType> clone;
169197
if (original_ == nullptr)
170198
return;
171199

172-
clone = original_->clone();
200+
std::shared_ptr<CacheType> clone = getClone();
201+
if (clone == nullptr)
202+
return;
173203

174204
// Lock cache
175205
std::unique_lock<std::mutex> lock(cache_mutex_);
176206
cache_.push_back(clone);
177207
}
178208

209+
std::shared_ptr<CacheType> getClone() const
210+
{
211+
std::shared_ptr<CacheType> clone;
212+
try
213+
{
214+
clone = original_->clone();
215+
}
216+
catch (std::exception& e)
217+
{
218+
CONSOLE_BRIDGE_logError("Clone Cache failed to update cache with the following exception: %s", e.what());
219+
return nullptr;
220+
}
221+
return clone;
222+
}
223+
179224
std::shared_ptr<CacheType> original_;
180225

181226
/** @brief The assigned cache size */

‎tesseract_common/test/clone_cache_unit.cpp‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,34 @@ class TestObjectSupportsUpdate : public TestObject
6363
}
6464
};
6565

66+
class TestObjectSupportsUpdateFailure : public TestObject
67+
{
68+
public:
69+
using Ptr = std::shared_ptr<TestObjectSupportsUpdateFailure>;
70+
using ConstPtr = std::shared_ptr<const TestObjectSupportsUpdateFailure>;
71+
72+
bool update(const TestObjectSupportsUpdateFailure::ConstPtr& pattern)
73+
{
74+
val_1 = pattern->val_1;
75+
val_2 = pattern->val_2;
76+
revision_ = pattern->revision_;
77+
return true;
78+
}
79+
80+
TestObjectSupportsUpdateFailure::Ptr clone() const
81+
{
82+
throw std::runtime_error("TestObjectSupportsUpdateFailure: clone failed!");
83+
}
84+
};
85+
6686
TEST(TesseractCloneCacheUnit, WithoutUpdate) // NOLINT
6787
{
6888
auto original = std::make_shared<TestObject>();
6989
original->val_1 = 1;
7090
original->val_2 = 2;
7191
auto clone_cache = std::make_shared<CloneCache<TestObject>>(original, 3);
92+
EXPECT_EQ(clone_cache->getCacheSize(), 3);
93+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 3);
7294

7395
// Baseline clone of original
7496
{
@@ -90,6 +112,27 @@ TEST(TesseractCloneCacheUnit, WithoutUpdate) // NOLINT
90112
original->revision_++;
91113
EXPECT_NE(original->val_1, clone->val_1);
92114
}
115+
// Cache should be empty so call update
116+
{
117+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 0);
118+
clone_cache->updateCache();
119+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 3);
120+
}
121+
// Now change revision and call update
122+
{
123+
original->revision_++;
124+
original->val_1 = 5;
125+
clone_cache->updateCache();
126+
auto clone = clone_cache->clone();
127+
EXPECT_EQ(original->val_1, clone->val_1);
128+
}
129+
// Now change revision and call clone
130+
{
131+
original->revision_++;
132+
original->val_1 = 6;
133+
auto clone = clone_cache->clone();
134+
EXPECT_EQ(original->val_1, clone->val_1);
135+
}
93136
// Try cloning more times than the cache is big
94137
for (int i = 0; i < 10; i++)
95138
{
@@ -99,6 +142,7 @@ TEST(TesseractCloneCacheUnit, WithoutUpdate) // NOLINT
99142
EXPECT_EQ(clone_cache->getCacheSize(), 3);
100143
clone_cache->setCacheSize(8);
101144
EXPECT_EQ(clone_cache->getCacheSize(), 8);
145+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 8);
102146
}
103147

104148
TEST(TesseractCloneCacheUnit, SupportsUpdate) // NOLINT
@@ -107,6 +151,8 @@ TEST(TesseractCloneCacheUnit, SupportsUpdate) // NOLINT
107151
original->val_1 = 1;
108152
original->val_2 = 2;
109153
auto clone_cache = std::make_shared<CloneCache<TestObjectSupportsUpdate>>(original, 3);
154+
EXPECT_EQ(clone_cache->getCacheSize(), 3);
155+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 3);
110156

111157
// Baseline clone of original
112158
{
@@ -127,6 +173,30 @@ TEST(TesseractCloneCacheUnit, SupportsUpdate) // NOLINT
127173

128174
original->revision_++;
129175
EXPECT_NE(original->val_1, clone->val_1);
176+
177+
auto updated_clone = clone_cache->clone();
178+
EXPECT_EQ(original->val_1, updated_clone->val_1);
179+
}
180+
// Cache should be empty so call update
181+
{
182+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 0);
183+
clone_cache->updateCache();
184+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 3);
185+
}
186+
// Now change revision and call update
187+
{
188+
original->revision_++;
189+
original->val_1 = 5;
190+
clone_cache->updateCache();
191+
auto clone = clone_cache->clone();
192+
EXPECT_EQ(original->val_1, clone->val_1);
193+
}
194+
// Now change revision and call clone
195+
{
196+
original->revision_++;
197+
original->val_1 = 6;
198+
auto clone = clone_cache->clone();
199+
EXPECT_EQ(original->val_1, clone->val_1);
130200
}
131201
// Try cloning more times than the cache is big
132202
for (int i = 0; i < 10; i++)
@@ -137,6 +207,42 @@ TEST(TesseractCloneCacheUnit, SupportsUpdate) // NOLINT
137207
EXPECT_EQ(clone_cache->getCacheSize(), 3);
138208
clone_cache->setCacheSize(8);
139209
EXPECT_EQ(clone_cache->getCacheSize(), 8);
210+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 8);
211+
}
212+
213+
TEST(TesseractCloneCacheUnit, SupportsUpdateFailure) // NOLINT
214+
{
215+
{ // Test original is a nullptr
216+
std::shared_ptr<TestObjectSupportsUpdate> original;
217+
auto clone_cache = std::make_shared<CloneCache<TestObjectSupportsUpdate>>(original, 3);
218+
EXPECT_TRUE(clone_cache->clone() == nullptr);
219+
EXPECT_EQ(clone_cache->getCacheSize(), 3);
220+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 0);
221+
clone_cache->updateCache();
222+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 0);
223+
}
224+
225+
// Test when clone throws an exception
226+
auto original = std::make_shared<TestObjectSupportsUpdateFailure>();
227+
original->val_1 = 1;
228+
original->val_2 = 2;
229+
auto clone_cache = std::make_shared<CloneCache<TestObjectSupportsUpdateFailure>>(original, 3);
230+
EXPECT_EQ(clone_cache->getCacheSize(), 3);
231+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 0);
232+
233+
// Baseline clone of original which should be nullptr because of exception during clone
234+
{
235+
EXPECT_TRUE(clone_cache->clone() == nullptr);
236+
EXPECT_EQ(clone_cache->getCacheSize(), 3);
237+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 0);
238+
}
239+
// Now it should update the cache and clone should be nullptr because of exception during clone
240+
{
241+
original->revision_++;
242+
EXPECT_EQ(clone_cache->getCacheSize(), 3);
243+
EXPECT_EQ(clone_cache->getCurrentCacheSize(), 0);
244+
EXPECT_TRUE(clone_cache->clone() == nullptr);
245+
}
140246
}
141247

142248
int main(int argc, char** argv)

0 commit comments

Comments
 (0)