Commit fc3463da by Jamie Madill Committed by Commit Bot

HandleAllocator: Fix heap ordering using std::greater.

The default heap ordering is to return the greatest element in the heap. Since the handle allocator expects a minimal return value on a new allocation, this caused a bug. The bug is triggered by reserving handles, allocating new handles, then freeing the handles and allocating again with the same allocator. Fix the bug by using std::greater instead of std::less, which will make the heap return the smallest value instead of largest. Also adds some logging debugging code for the handle allocators. Bug: angleproject:1458 Change-Id: Ibef5dcbed0a664ccad0e0335f081e2355162584b Reviewed-on: https://chromium-review.googlesource.com/848644 Commit-Queue: Jamie Madill <jmadill@chromium.org> Reviewed-by: 's avatarFrank Henigman <fjhenigman@chromium.org>
parent 36937a64
...@@ -10,6 +10,7 @@ ...@@ -10,6 +10,7 @@
#include "libANGLE/HandleAllocator.h" #include "libANGLE/HandleAllocator.h"
#include <algorithm> #include <algorithm>
#include <functional>
#include "common/debug.h" #include "common/debug.h"
...@@ -24,7 +25,7 @@ struct HandleAllocator::HandleRangeComparator ...@@ -24,7 +25,7 @@ struct HandleAllocator::HandleRangeComparator
} }
}; };
HandleAllocator::HandleAllocator() : mBaseValue(1), mNextValue(1) HandleAllocator::HandleAllocator() : mBaseValue(1), mNextValue(1), mLoggingEnabled(false)
{ {
mUnallocatedList.push_back(HandleRange(1, std::numeric_limits<GLuint>::max())); mUnallocatedList.push_back(HandleRange(1, std::numeric_limits<GLuint>::max()));
} }
...@@ -52,9 +53,15 @@ GLuint HandleAllocator::allocate() ...@@ -52,9 +53,15 @@ GLuint HandleAllocator::allocate()
// Allocate from released list, logarithmic time for pop_heap. // Allocate from released list, logarithmic time for pop_heap.
if (!mReleasedList.empty()) if (!mReleasedList.empty())
{ {
std::pop_heap(mReleasedList.begin(), mReleasedList.end()); std::pop_heap(mReleasedList.begin(), mReleasedList.end(), std::greater<GLuint>());
GLuint reusedHandle = mReleasedList.back(); GLuint reusedHandle = mReleasedList.back();
mReleasedList.pop_back(); mReleasedList.pop_back();
if (mLoggingEnabled)
{
WARN() << "HandleAllocator::allocate reusing " << reusedHandle << std::endl;
}
return reusedHandle; return reusedHandle;
} }
...@@ -73,18 +80,33 @@ GLuint HandleAllocator::allocate() ...@@ -73,18 +80,33 @@ GLuint HandleAllocator::allocate()
listIt->begin++; listIt->begin++;
} }
if (mLoggingEnabled)
{
WARN() << "HandleAllocator::allocate allocating " << freeListHandle << std::endl;
}
return freeListHandle; return freeListHandle;
} }
void HandleAllocator::release(GLuint handle) void HandleAllocator::release(GLuint handle)
{ {
if (mLoggingEnabled)
{
WARN() << "HandleAllocator::release releasing " << handle << std::endl;
}
// Add to released list, logarithmic time for push_heap. // Add to released list, logarithmic time for push_heap.
mReleasedList.push_back(handle); mReleasedList.push_back(handle);
std::push_heap(mReleasedList.begin(), mReleasedList.end()); std::push_heap(mReleasedList.begin(), mReleasedList.end(), std::greater<GLuint>());
} }
void HandleAllocator::reserve(GLuint handle) void HandleAllocator::reserve(GLuint handle)
{ {
if (mLoggingEnabled)
{
WARN() << "HandleAllocator::reserve reserving " << handle << std::endl;
}
// Clear from released list -- might be a slow operation. // Clear from released list -- might be a slow operation.
if (!mReleasedList.empty()) if (!mReleasedList.empty())
{ {
...@@ -139,4 +161,9 @@ void HandleAllocator::reset() ...@@ -139,4 +161,9 @@ void HandleAllocator::reset()
mNextValue = 1; mNextValue = 1;
} }
void HandleAllocator::enableLogging(bool enabled)
{
mLoggingEnabled = enabled;
}
} // namespace gl } // namespace gl
...@@ -34,6 +34,8 @@ class HandleAllocator final : angle::NonCopyable ...@@ -34,6 +34,8 @@ class HandleAllocator final : angle::NonCopyable
void reserve(GLuint handle); void reserve(GLuint handle);
void reset(); void reset();
void enableLogging(bool enabled);
private: private:
GLuint mBaseValue; GLuint mBaseValue;
GLuint mNextValue; GLuint mNextValue;
...@@ -56,6 +58,8 @@ class HandleAllocator final : angle::NonCopyable ...@@ -56,6 +58,8 @@ class HandleAllocator final : angle::NonCopyable
// released, stored in a heap. // released, stored in a heap.
std::vector<HandleRange> mUnallocatedList; std::vector<HandleRange> mUnallocatedList;
std::vector<GLuint> mReleasedList; std::vector<GLuint> mReleasedList;
bool mLoggingEnabled;
}; };
} // namespace gl } // namespace gl
......
...@@ -150,4 +150,24 @@ TEST(HandleAllocatorTest, Reset) ...@@ -150,4 +150,24 @@ TEST(HandleAllocatorTest, Reset)
} }
} }
// Covers a particular bug with reserving and allocating sub ranges.
TEST(HandleAllocatorTest, ReserveAndAllocateIterated)
{
gl::HandleAllocator allocator;
for (int iteration = 0; iteration < 3; ++iteration)
{
allocator.reserve(5);
allocator.reserve(6);
GLuint a = allocator.allocate();
GLuint b = allocator.allocate();
GLuint c = allocator.allocate();
allocator.release(c);
allocator.release(a);
allocator.release(b);
allocator.release(5);
allocator.release(6);
}
}
} // anonymous namespace } // anonymous namespace
...@@ -260,6 +260,11 @@ void TextureManager::signalAllTexturesDirty() const ...@@ -260,6 +260,11 @@ void TextureManager::signalAllTexturesDirty() const
} }
} }
void TextureManager::enableHandleAllocatorLogging()
{
mHandleAllocator.enableLogging(true);
}
// RenderbufferManager Implementation. // RenderbufferManager Implementation.
// static // static
......
...@@ -167,6 +167,8 @@ class TextureManager : public TypedResourceManager<Texture, HandleAllocator, Tex ...@@ -167,6 +167,8 @@ class TextureManager : public TypedResourceManager<Texture, HandleAllocator, Tex
static Texture *AllocateNewObject(rx::GLImplFactory *factory, GLuint handle, GLenum target); static Texture *AllocateNewObject(rx::GLImplFactory *factory, GLuint handle, GLenum target);
static void DeleteObject(const Context *context, Texture *texture); static void DeleteObject(const Context *context, Texture *texture);
void enableHandleAllocatorLogging();
protected: protected:
~TextureManager() override {} ~TextureManager() override {}
}; };
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment