Skip to content

Commit 205ef43

Browse files
kmusethkmuseth
authored
Merge pull request #858 from kmuseth/fix_PagedArray
Fixed crash in PagedArray unit test
2 parents 1bf3c6e + 5a8a982 commit 205ef43

File tree

4 files changed

+48
-245
lines changed

4 files changed

+48
-245
lines changed

‎CHANGES‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ Version 7.2.0 - In Development
3232
Bug fixes:
3333
- Fixed a bug which could cause recursive compile time instantiations of
3434
TypeList objects, manifesting in longer compile times.
35+
- Deprecated util::PagedArray::push_back due to a race condition. Instead
36+
use util::PagedArray::ValueBuffer::push_back which is faster and thread-safe.
3537

3638
API changes:
3739
- Deprecated tree::LeafManager::getNodes. This method is no longer used when

‎doc/changes.txt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,10 @@ Houdini:
4545
Bug Fixes:
4646
- Fixed a bug which could cause recursive compile time instantiations of
4747
TypeList objects, manifesting in longer compile times.
48+
- Deprecated @vdblink::util::PagedArray::push_back util::PagedArray::push_back@endlink
49+
due to a race condition. Instead use
50+
@vdblink::util::PagedArray::ValueBuffer::push_back util::PagedArray::ValueBuffer::push_back@endlink
51+
which is faster and thread-safe.
4852

4953
@par
5054
API changes:

‎openvdb/openvdb/unittest/TestUtil.cc‎

Lines changed: 22 additions & 205 deletions
Original file line numberDiff line numberDiff line change
@@ -32,35 +32,20 @@ class TestUtil: public CppUnit::TestCase
3232
CPPUNIT_TEST(testFormats);
3333
CPPUNIT_TEST(testCpuTimer);
3434
CPPUNIT_TEST(testPagedArray);
35-
CPPUNIT_TEST(testPagedArrayPushBack);
3635
CPPUNIT_TEST_SUITE_END();
3736

3837
void testCpuTimer();
3938
void testFormats();
4039
void testPagedArray();
41-
void testPagedArrayPushBack();
4240

4341
using RangeT = tbb::blocked_range<size_t>;
4442

45-
// Multi-threading ArrayT::push_back
46-
template<typename ArrayT>
47-
struct ArrayPushBack {
48-
ArrayPushBack(ArrayT& array) : mArray(&array) {}
49-
void parallel(size_t size) {tbb::parallel_for(RangeT(size_t(0), size, mArray->pageSize()), *this);}
50-
void serial(size_t size) { (*this)(RangeT(size_t(0), size)); }
51-
void unsafe(size_t size) { for (size_t i=0; i!=size; ++i) mArray->push_back_unsafe(i); }
52-
void operator()(const RangeT& r) const {
53-
for (size_t i=r.begin(), n=r.end(); i!=n; ++i) mArray->push_back(i);
54-
}
55-
ArrayT* mArray;
56-
};
57-
5843
// Multi-threading ArrayT::ValueBuffer::push_back
5944
template<typename ArrayT>
6045
struct BufferPushBack {
6146
BufferPushBack(ArrayT& array) : mBuffer(array) {}
6247
void parallel(size_t size) {
63-
tbb::parallel_for(RangeT(size_t(0), size, mBuffer.parent().pageSize()), *this);
48+
tbb::parallel_for(RangeT(size_t(0), size, 256*mBuffer.pageSize()), *this);
6449
}
6550
void serial(size_t size) { (*this)(RangeT(size_t(0), size)); }
6651
void operator()(const RangeT& r) const {
@@ -77,7 +62,7 @@ class TestUtil: public CppUnit::TestCase
7762
void parallel(size_t size) {
7863
typename ArrayT::ValueBuffer exemplar(*mArray);//dummy used for initialization
7964
mPool = new PoolT(exemplar);//thread local storage pool of ValueBuffers
80-
tbb::parallel_for(RangeT(size_t(0), size, mArray->pageSize()), *this);
65+
tbb::parallel_for(RangeT(size_t(0), size, 256*mArray->pageSize()), *this);
8166
for (auto i=mPool->begin(); i!=mPool->end(); ++i) i->flush();
8267
delete mPool;
8368
}
@@ -170,57 +155,6 @@ TestUtil::testCpuTimer()
170155
}
171156
}
172157

173-
void
174-
TestUtil::testPagedArrayPushBack()
175-
{
176-
#ifdef BENCHMARK_PAGED_ARRAY
177-
const size_t problemSize = 256000;
178-
openvdb::util::CpuTimer timer;
179-
std::cerr << "\nProblem size for benchmark: " << problemSize << std::endl;
180-
#else
181-
const size_t problemSize = 256000;
182-
#endif
183-
{//parallel PagedArray::push_back
184-
using ArrayT = openvdb::util::PagedArray<size_t>;
185-
ArrayT d;
186-
#ifdef BENCHMARK_PAGED_ARRAY
187-
timer.start("3: Parallel PagedArray::push_back with default page size");
188-
#endif
189-
{// for some reason this:
190-
ArrayPushBack<ArrayT> tmp(d);
191-
tmp.parallel(problemSize);
192-
}// is faster than:
193-
//tbb::parallel_for(tbb::blocked_range<size_t>(0, problemSize, d.pageSize()),
194-
// [&d](const tbb::blocked_range<size_t> &range){
195-
// for (size_t i=range.begin(); i!=range.end(); ++i) d.push_back(i);});
196-
#ifdef BENCHMARK_PAGED_ARRAY
197-
timer.stop();
198-
#endif
199-
CPPUNIT_ASSERT_EQUAL(size_t(10), d.log2PageSize());
200-
CPPUNIT_ASSERT_EQUAL(size_t(1)<<d.log2PageSize(), d.pageSize());
201-
CPPUNIT_ASSERT_EQUAL(problemSize, d.size());
202-
// pageCount - 1 = max index >> log2PageSize
203-
CPPUNIT_ASSERT_EQUAL((d.size()-1)>>d.log2PageSize(), d.pageCount()-1);
204-
CPPUNIT_ASSERT_EQUAL(d.pageCount()*d.pageSize(), d.capacity());
205-
206-
#ifdef BENCHMARK_PAGED_ARRAY
207-
timer.start("parallel sort with a default page size");
208-
#endif
209-
d.sort();
210-
#ifdef BENCHMARK_PAGED_ARRAY
211-
timer.stop();
212-
#endif
213-
for (size_t i=0, n=d.size(); i<n; ++i) CPPUNIT_ASSERT_EQUAL(i, d[i]);
214-
215-
CPPUNIT_ASSERT_EQUAL(problemSize, d.push_back(1));
216-
CPPUNIT_ASSERT_EQUAL(problemSize+1, d.size());
217-
CPPUNIT_ASSERT_EQUAL(size_t(1)<<d.log2PageSize(), d.pageSize());
218-
// pageCount - 1 = max index >> log2PageSize
219-
CPPUNIT_ASSERT_EQUAL((d.size()-1)>>d.log2PageSize(), d.pageCount()-1);
220-
CPPUNIT_ASSERT_EQUAL(d.pageCount()*d.pageSize(), d.capacity());
221-
}
222-
}
223-
224158
void
225159
TestUtil::testPagedArray()
226160
{
@@ -233,158 +167,51 @@ TestUtil::testPagedArray()
233167
#endif
234168

235169
{//serial PagedArray::push_back (check return value)
236-
openvdb::util::PagedArray<int, size_t(8)> d;
170+
openvdb::util::PagedArray<int> d;
171+
237172
CPPUNIT_ASSERT(d.isEmpty());
238173
CPPUNIT_ASSERT_EQUAL(size_t(0), d.size());
239-
CPPUNIT_ASSERT_EQUAL(size_t(8), d.log2PageSize());
174+
CPPUNIT_ASSERT_EQUAL(size_t(10), d.log2PageSize());
240175
CPPUNIT_ASSERT_EQUAL(size_t(1)<<d.log2PageSize(), d.pageSize());
241176
CPPUNIT_ASSERT_EQUAL(size_t(0), d.pageCount());
242177
CPPUNIT_ASSERT_EQUAL(size_t(0), d.capacity());
243178

244-
CPPUNIT_ASSERT_EQUAL(size_t(0), d.push_back(10));
179+
CPPUNIT_ASSERT_EQUAL(size_t(0), d.push_back_unsafe(10));
245180
CPPUNIT_ASSERT_EQUAL(10, d[0]);
246181
CPPUNIT_ASSERT(!d.isEmpty());
247182
CPPUNIT_ASSERT_EQUAL(size_t(1), d.size());
248183
CPPUNIT_ASSERT_EQUAL(size_t(1), d.pageCount());
249184
CPPUNIT_ASSERT_EQUAL(d.pageSize(), d.capacity());
250185

251-
CPPUNIT_ASSERT_EQUAL(size_t(1), d.push_back(1));
186+
CPPUNIT_ASSERT_EQUAL(size_t(1), d.push_back_unsafe(1));
252187
CPPUNIT_ASSERT_EQUAL(size_t(2), d.size());
253188
CPPUNIT_ASSERT_EQUAL(size_t(1), d.pageCount());
254189
CPPUNIT_ASSERT_EQUAL(d.pageSize(), d.capacity());
255190

256-
for (size_t i=2; i<d.pageSize(); ++i) CPPUNIT_ASSERT_EQUAL(i, d.push_back(int(i)));
191+
for (size_t i=2; i<d.pageSize(); ++i) CPPUNIT_ASSERT_EQUAL(i, d.push_back_unsafe(int(i)));
257192
CPPUNIT_ASSERT_EQUAL(d.pageSize(), d.size());
258193
CPPUNIT_ASSERT_EQUAL(size_t(1), d.pageCount());
259194
CPPUNIT_ASSERT_EQUAL(d.pageSize(), d.capacity());
260195

261196
for (int i=2, n=int(d.size()); i<n; ++i) CPPUNIT_ASSERT_EQUAL(i, d[i]);
262197

263-
CPPUNIT_ASSERT_EQUAL(d.pageSize(), d.push_back(1));
198+
CPPUNIT_ASSERT_EQUAL(d.pageSize(), d.push_back_unsafe(1));
264199
CPPUNIT_ASSERT_EQUAL(d.pageSize()+1, d.size());
265200
CPPUNIT_ASSERT_EQUAL(size_t(2), d.pageCount());
266201
CPPUNIT_ASSERT_EQUAL(2*d.pageSize(), d.capacity());
267202
}
268-
{//serial PagedArray::push_back
269-
using ArrayT = openvdb::util::PagedArray<size_t>;
270-
ArrayT d;
271-
#ifdef BENCHMARK_PAGED_ARRAY
272-
timer.start("1: Serial PagedArray::push_back with default page size");
273-
#endif
274-
{// for some reason this:
275-
ArrayPushBack<ArrayT> tmp(d);
276-
tmp.serial(problemSize);
277-
}// is faster than:
278-
//for (size_t i=0; i<problemSize; ++i) d.push_back(i);
279-
#ifdef BENCHMARK_PAGED_ARRAY
280-
timer.stop();
281-
#endif
282-
CPPUNIT_ASSERT_EQUAL(size_t(10), d.log2PageSize());
283-
CPPUNIT_ASSERT_EQUAL(size_t(1)<<d.log2PageSize(), d.pageSize());
284-
CPPUNIT_ASSERT_EQUAL(problemSize, d.size());
285-
// pageCount - 1 = max index >> log2PageSize
286-
CPPUNIT_ASSERT_EQUAL((d.size()-1)>>d.log2PageSize(), d.pageCount()-1);
287-
CPPUNIT_ASSERT_EQUAL(d.pageCount()*d.pageSize(), d.capacity());
288-
for (size_t i=0, n=d.size(); i<n; ++i) CPPUNIT_ASSERT_EQUAL(i, d[i]);
289-
}
290203
{//serial PagedArray::push_back_unsafe
291-
using ArrayT = openvdb::util::PagedArray<size_t>;
292-
ArrayT d;
293204
#ifdef BENCHMARK_PAGED_ARRAY
294205
timer.start("2: Serial PagedArray::push_back_unsafe with default page size");
295206
#endif
296-
{// for some reason this:
297-
ArrayPushBack<ArrayT> tmp(d);
298-
tmp.unsafe(problemSize);
299-
}// is faster than:
300-
//openvdb::util::PagedArray<size_t> d;
301-
//for (size_t i=0; i<problemSize; ++i) d.push_back_unsafe(i);
207+
openvdb::util::PagedArray<size_t> d;
208+
for (size_t i=0; i<problemSize; ++i) d.push_back_unsafe(i);
302209
#ifdef BENCHMARK_PAGED_ARRAY
303210
timer.stop();
304211
#endif
305212
CPPUNIT_ASSERT_EQUAL(problemSize, d.size());
306213
for (size_t i=0; i<problemSize; ++i) CPPUNIT_ASSERT_EQUAL(i, d[i]);
307214
}
308-
{//parallel PagedArray::push_back
309-
using ArrayT = openvdb::util::PagedArray<size_t>;
310-
ArrayT d;
311-
#ifdef BENCHMARK_PAGED_ARRAY
312-
timer.start("3: Parallel PagedArray::push_back with default page size");
313-
#endif
314-
//{// for some reason this:
315-
// ArrayPushBack<ArrayT> tmp(d);
316-
// tmp.parallel(problemSize);
317-
//}// is faster than:
318-
tbb::parallel_for(tbb::blocked_range<size_t>(0, problemSize, d.pageSize()),
319-
[&d](const tbb::blocked_range<size_t> &range){
320-
for (size_t i=range.begin(); i!=range.end(); ++i) d.push_back(i);});
321-
#ifdef BENCHMARK_PAGED_ARRAY
322-
timer.stop();
323-
#endif
324-
CPPUNIT_ASSERT_EQUAL(size_t(10), d.log2PageSize());
325-
CPPUNIT_ASSERT_EQUAL(size_t(1)<<d.log2PageSize(), d.pageSize());
326-
CPPUNIT_ASSERT_EQUAL(problemSize, d.size());
327-
// pageCount - 1 = max index >> log2PageSize
328-
CPPUNIT_ASSERT_EQUAL((d.size()-1)>>d.log2PageSize(), d.pageCount()-1);
329-
CPPUNIT_ASSERT_EQUAL(d.pageCount()*d.pageSize(), d.capacity());
330-
331-
#ifdef BENCHMARK_PAGED_ARRAY
332-
timer.start("parallel sort with a default page size");
333-
#endif
334-
d.sort();
335-
#ifdef BENCHMARK_PAGED_ARRAY
336-
timer.stop();
337-
#endif
338-
for (size_t i=0, n=d.size(); i<n; ++i) CPPUNIT_ASSERT_EQUAL(i, d[i]);
339-
340-
CPPUNIT_ASSERT_EQUAL(problemSize, d.push_back(1));
341-
CPPUNIT_ASSERT_EQUAL(problemSize+1, d.size());
342-
CPPUNIT_ASSERT_EQUAL(size_t(1)<<d.log2PageSize(), d.pageSize());
343-
// pageCount - 1 = max index >> log2PageSize
344-
CPPUNIT_ASSERT_EQUAL((d.size()-1)>>d.log2PageSize(), d.pageCount()-1);
345-
CPPUNIT_ASSERT_EQUAL(d.pageCount()*d.pageSize(), d.capacity());
346-
347-
348-
}
349-
{//parallel PagedArray::push_back with page size of only 8
350-
using ArrayT = openvdb::util::PagedArray<size_t, size_t(3)>;
351-
ArrayT d;
352-
#ifdef BENCHMARK_PAGED_ARRAY
353-
timer.start("4: Parallel PagedArray::push_back with page size of only 8");
354-
#endif
355-
{// for some reason this:
356-
ArrayPushBack<ArrayT> tmp(d);
357-
tmp.parallel(problemSize);
358-
}// is faster than:
359-
//tbb::parallel_for(tbb::blocked_range<size_t>(0, problemSize, d.pageSize()),
360-
// [&d](const tbb::blocked_range<size_t> &range){
361-
// for (size_t i=range.begin(); i!=range.end(); ++i) d.push_back(i);});
362-
#ifdef BENCHMARK_PAGED_ARRAY
363-
timer.stop();
364-
#endif
365-
CPPUNIT_ASSERT_EQUAL(size_t(3), d.log2PageSize());
366-
CPPUNIT_ASSERT_EQUAL(size_t(1)<<d.log2PageSize(), d.pageSize());
367-
CPPUNIT_ASSERT_EQUAL(problemSize, d.size());
368-
// pageCount - 1 = max index >> log2PageSize
369-
CPPUNIT_ASSERT_EQUAL((d.size()-1)>>d.log2PageSize(), d.pageCount()-1);
370-
CPPUNIT_ASSERT_EQUAL(d.pageCount()*d.pageSize(), d.capacity());
371-
372-
#ifdef BENCHMARK_PAGED_ARRAY
373-
timer.start("parallel sort with a page size of only 8");
374-
#endif
375-
d.sort();
376-
#ifdef BENCHMARK_PAGED_ARRAY
377-
timer.stop();
378-
#endif
379-
for (size_t i=0, n=d.size(); i<n; ++i) CPPUNIT_ASSERT_EQUAL(i, d[i]);
380-
381-
CPPUNIT_ASSERT_EQUAL(problemSize, d.push_back(1));
382-
CPPUNIT_ASSERT_EQUAL(problemSize+1, d.size());
383-
CPPUNIT_ASSERT_EQUAL(size_t(1)<<d.log2PageSize(), d.pageSize());
384-
// pageCount - 1 = max index >> log2PageSize
385-
CPPUNIT_ASSERT_EQUAL((d.size()-1)>>d.log2PageSize(), d.pageCount()-1);
386-
CPPUNIT_ASSERT_EQUAL(d.pageCount()*d.pageSize(), d.capacity());
387-
}
388215
#ifdef BENCHMARK_PAGED_ARRAY
389216
{//benchmark against a std::vector
390217
timer.start("5: Serial std::vector::push_back");
@@ -429,7 +256,7 @@ TestUtil::testPagedArray()
429256
#endif
430257

431258
{//serial PagedArray::ValueBuffer::push_back
432-
using ArrayT = openvdb::util::PagedArray<size_t>;
259+
using ArrayT = openvdb::util::PagedArray<size_t, 3UL>;
433260
ArrayT d;
434261

435262
CPPUNIT_ASSERT_EQUAL(size_t(0), d.size());
@@ -445,13 +272,8 @@ TestUtil::testPagedArray()
445272
#ifdef BENCHMARK_PAGED_ARRAY
446273
timer.start("9: Serial PagedArray::ValueBuffer::push_back");
447274
#endif
448-
{// for some reason this:
449-
BufferPushBack<ArrayT> tmp(d);
450-
tmp.serial(problemSize);
451-
// is faster than:
452-
//ArrayT::ValueBuffer buffer(d);
453-
//for (size_t i=0, n=problemSize; i<n; ++i) buffer.push_back(i);
454-
}
275+
BufferPushBack<ArrayT> tmp(d);
276+
tmp.serial(problemSize);
455277

456278
#ifdef BENCHMARK_PAGED_ARRAY
457279
timer.stop();
@@ -486,14 +308,8 @@ TestUtil::testPagedArray()
486308
#ifdef BENCHMARK_PAGED_ARRAY
487309
timer.start("10: Parallel PagedArray::ValueBuffer::push_back");
488310
#endif
489-
{// for some reason this:
490-
BufferPushBack<ArrayT> tmp(d);
491-
tmp.parallel(problemSize);
492-
}// is faster than:
493-
//tbb::parallel_for(tbb::blocked_range<size_t>(0, problemSize, d.pageSize()),
494-
// [&d](const tbb::blocked_range<size_t> &r){
495-
// typename ArrayT::ValueBuffer buffer(d);
496-
// for (size_t i=r.begin(), n=r.end(); i!=n; ++i) buffer.push_back(i);});
311+
BufferPushBack<ArrayT> tmp(d);
312+
tmp.parallel(problemSize);
497313
#ifdef BENCHMARK_PAGED_ARRAY
498314
timer.stop();
499315
#endif
@@ -522,7 +338,7 @@ TestUtil::testPagedArray()
522338
#endif
523339
for (size_t i=0, n=d.size()-1; i<=n; ++i) CPPUNIT_ASSERT_EQUAL(n-i, d[i]);
524340

525-
CPPUNIT_ASSERT_EQUAL(problemSize, d.push_back(1));
341+
CPPUNIT_ASSERT_EQUAL(problemSize, d.push_back_unsafe(1));
526342
CPPUNIT_ASSERT_EQUAL(problemSize+1, d.size());
527343
CPPUNIT_ASSERT_EQUAL(size_t(1)<<d.log2PageSize(), d.pageSize());
528344
// pageCount - 1 = max index >> log2PageSize
@@ -539,7 +355,8 @@ TestUtil::testPagedArray()
539355
ArrayT d;
540356
CPPUNIT_ASSERT_EQUAL(size_t(0), d.size());
541357
{
542-
ArrayT::ValueBuffer vc(d);
358+
//ArrayT::ValueBuffer vc(d);
359+
auto vc = d.getBuffer();
543360
vc.push_back(1);
544361
vc.push_back(2);
545362
CPPUNIT_ASSERT_EQUAL(size_t(0), d.size());
@@ -607,7 +424,7 @@ TestUtil::testPagedArray()
607424
CPPUNIT_ASSERT_EQUAL((d.size()-1)>>d.log2PageSize(), d.pageCount()-1);
608425
CPPUNIT_ASSERT_EQUAL(d.pageCount()*d.pageSize(), d.capacity());
609426
CPPUNIT_ASSERT(!d.isPartiallyFull());
610-
d.push_back(problemSize);
427+
d.push_back_unsafe(problemSize);
611428
CPPUNIT_ASSERT(d.isPartiallyFull());
612429

613430
tbb::parallel_for(tbb::blocked_range<size_t>(problemSize+1, 2*problemSize+1, d2.pageSize()),
@@ -649,9 +466,9 @@ TestUtil::testPagedArray()
649466
for (size_t i=0, n=d.size(); i<n; ++i) CPPUNIT_ASSERT_EQUAL(i, d[i]);
650467
}
651468
{//examples in doxygen
652-
{//1
469+
{// 1
653470
openvdb::util::PagedArray<int> array;
654-
for (int i=0; i<100000; ++i) array.push_back(i);
471+
for (int i=0; i<100000; ++i) array.push_back_unsafe(i);
655472
for (int i=0; i<100000; ++i) CPPUNIT_ASSERT_EQUAL(i, array[i]);
656473
}
657474
{//2A

0 commit comments

Comments
 (0)