1008717 - WebGLElementArrayCache cleanup. In particular, less raw pointer manipulation.

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: webglelementarraycache

This patch does the following things. I know it's bad form for a patch to do multiple things, but it's not very big, and it's tested (TestWebGLElementArrayCache in valgrind, so we can be pretty sure that it's correct) so that should help with reviewing.

1) Use ScopedDeletePtr to hold the 3 Tree's (instead of raw pointers).
2) Use nsTArray<uint8_t> to hold the untyped data butter (instead of raw void* pointer).
3) Rename that array from "mUntypedData" to "mBytes" since it's now typed as bytes (uint8_t).
4) The mByteSize member goes away, one now uses mBytes.Length().
5) General renaming: size --> length, accordingly. It's bad to have two different terms for the same thing, and nsTArray chose "Length()". Note that we were already using nsTArray for mTreeData before, so we had the term "Length" already.
6) The mParentByteSize member is renamed to mSupportedParentByteLength.  The "Supported" part here is to make it clear that that is what the tree supports i.e. what the tree's own data structures expect the parent to be like. This could be different from what the parent actually is, in which case the tree is invalid.
7) Correctly propagate error results from BufferSubData to BufferData
8) Correctly propagate error results from the WebGLElementArrayCacheTree constructor.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

bjacob:/hack/mozilla-inbound$ hg diff --stat -c tip
 content/canvas/src/WebGLElementArrayCache.cpp |  101 ++++++++++++++++++++------------------
 content/canvas/src/WebGLElementArrayCache.h   |   29 +++-------
 2 files changed, 64 insertions(+), 66 deletions(-)

^ Negative net change.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Note in particular that the change from void* pointers and realloc() calls, to using nsTArray<uint8_t> and SetLength(), would have averted the trouble I had on 984783 with realloc behaving like free() and subsequent realloc() being use-after-free's that only ASan / valgrind would catch.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Does this depend on bug 984783?

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Sure, but it doesn't matter now that it's landed.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8420651 [details] [diff] [review]
webglelementarraycache

Review of attachment 8420651 [details] [diff] [review]:
-----------------------------------------------------------------

I need a better explanation for mSupportedParentByteLength.

::: content/canvas/src/WebGLElementArrayCache.cpp
@@ +144,5 @@
>  private:
>    WebGLElementArrayCache& mParent;
>    FallibleTArray<T> mTreeData;
>    size_t mNumLeaves;
> +  size_t mSupportedParentByteLength;

I'm still a little unclear on this. If it's the capacity, can we call it 'capacity'?
Can we give this a comment that says what it's for? I really don't feel like I understand it properly, so I'm having trouble doing this review.

@@ +161,3 @@
>      }
>    }
> +  

Death to EOL whitespace, please!

@@ +319,5 @@
>  
>  template<>
>  struct TreeForType<uint8_t>
>  {
> +  static ScopedDeletePtr<WebGLElementArrayCacheTree<uint8_t>>& Run(WebGLElementArrayCache *b) { return b->mUint8Tree; }

Can we do this on 2+ lines?

@@ +475,5 @@
> +}
> +
> +bool WebGLElementArrayCache::BufferSubData(size_t pos, const void* ptr, size_t updateByteLength) {
> +  MOZ_ASSERT(pos + updateByteLength <= mBytes.Length());
> +  if (!updateByteLength) return true;

Can we not have this on one line?

@@ +480,2 @@
>    if (ptr)
> +      memcpy(mBytes.Elements() + pos, ptr, updateByteLength);

4-space in 2-space land

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Comment on attachment 8420651 [details] [diff] [review]
> webglelementarraycache
> 
> Review of attachment 8420651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I need a better explanation for mSupportedParentByteLength.
> 
> ::: content/canvas/src/WebGLElementArrayCache.cpp
> @@ +144,5 @@
> >  private:
> >    WebGLElementArrayCache& mParent;
> >    FallibleTArray<T> mTreeData;
> >    size_t mNumLeaves;
> > +  size_t mSupportedParentByteLength;
> 
> I'm still a little unclear on this. If it's the capacity, can we call it
> 'capacity'?
> Can we give this a comment that says what it's for? I really don't feel like
> I understand it properly, so I'm having trouble doing this review.

It is a length, not a capacity.

The WebGLElementArrayCache (mParent) holds a buffer, the plain WebGL buffer bytes. That's mParent.mBytes.

Here we are in WebGLElementArrayCacheTree, which holds its own tree storage (mTreeData). This tree storage has a length that is computed from mParent.mBytes.Length(). At any given time, it assumes that mParent.mBytes.Length() is still the same as when it was last resized.

When we rely on an assumption like this, it's valuable to know explicitly what we are assuming, so that an external entity (the parent WebGLElementArrayCache) can verify that our assumption is correct. That is the role of mSupportedParentByteLength. It tells what parent byte length this tree is assuming. The parent can compare it to its own byte length to see if the tree is correctly sized. The tree can perform the same comparison in Update() to see if it needs to resize its storage.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: webglelementarraycache

Addressed nits, added comments on data members; and while adding these comments, realized that mNumLeaves was a useless redundant member because, as the class comment says, it is always equal to half of mTreeData.Length(). Removed mNumLeaves, added a NumLeaves() getter. Also const-qualified a few such getters.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Note: verified again to run free of errors in valgrind.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: webglelementarraycache

Actually... there was a good reason why mSupportedParentByteLength was hard to name and describe adequately, and why it made you feel uncomfortable: it was unnecessary complication. This new patch removes it. The nontrivial change that was necessary to achieve that, is that now the WebGLElementArrayCacheTree constructor does not attempt to initialize the tree. The caller has to call Update() and handle error on its side. There's exactly one call site, and this patch adapts it so.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Note: verified again in valgrind.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: webglelementarraycache

Moar cleanup: removed the partial-unrolling-by-8 optimization, strict code-removal diff.

Reason: I dont remember validating that optimization by careful benchmarking, and I just noticed that the Step #1 loop, that processes the buffer contents and writes into the tree leaves, which is the most expensive loop by far, is not partially unrolled anyway, so having this partial unrolling here seems of dubious usefulness.

Also, compilers are increasingly good at partial unrolling and this case is an easy, typical one for them to partially unroll automatically.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8422522 [details] [diff] [review]
webglelementarraycache

Review of attachment 8422522 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLElementArrayCache.cpp
@@ +356,5 @@
> +  size_t numberOfElements = mParent.mBytes.Length() / sizeof(T);
> +  size_t requiredNumLeaves =
> +    numberOfElements > 0
> +    ? NextPowerOfTwo((numberOfElements + sElementsPerLeaf - 1) / sElementsPerLeaf)
> +    : 0;

I don't love this formatting.

Wouldn't this be cleaner as:
size_t requiredNumLeaves = 0;
if (numberOfElements > 0) {
  // Comment explaining the next line's math, whose reasoning escapes me.
  size_t descriptiveVariableName = (numberOfElements + sElementsPerLeaf - 1) / sElementsPerLeaf;
  requiredNumLeaves = NextPowerOfTwo(descriptiveVariableName);
}

@@ +511,5 @@
>    if (!tree) {
>      tree = new WebGLElementArrayCacheTree<T>(*this);
> +    if (mBytes.Length()) {
> +      bool valid = tree->Update(0, mBytes.Length() - 1);
> +      if (!valid) {

Is this something we should ASSERT on?

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1bd8b0ef2a

I've answered your two questions in comments in this cset.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3e1bd8b0ef2a