Use updateData instead of re-creating buffers for repopulated paint arrays

[ChrisLoer]

Fixes issue #6839 (memory leak on frequent calls to setFeatureState). The leak was caused by mismatched calls to "createVertexBuffer" and "destroy". We didn't see the leak on most machines, presumably due to differences in how different GPU drivers handled re-creating the buffers. @ryanbaumann could reproduce the problem on one of his Windows machines, and verified the problem went away with these changes.

Improves performance of PaintStates benchmark. The performance improvement is presumably because we're able to take advantage of the DYNAMIC_DRAW option intended for frequently updated buffers. I ran the benchmarks multiple times and saw the same pattern: it looks like the PaintStates benchmark has much less variability after this change (and it looks like before the change there was actually a pattern in which progressive iterations of the bench seemed to slow down -- note the way the dots slope up to the right).

There are a couple things that bothered me a little but don't seem too serious:

• Duplication between CompositeExpressionBinder and SourceExpressionBinder
• Peaking inside the VertexBuffer to test if it's "live" (e.g. this.paintVertexBuffer.buffer) -- it kind of feels like this should be handled by VertexBuffer somehow. If buffer is null, that means destroy() has already been called, and it needs to be re-recreated (I don't actually know of a pathway that would cause this to happen).
• We call updateData regardless of whether DYNAMIC_DRAW is enabled (it's cued off expression.isStateDependent). But I believe updateData is still fine if it ends up getting called on a STATIC_DRAW buffer (should be no worse than destroying and re-creating the buffer).

Launch Checklist

• briefly describe the changes in this PR
• write tests for all new functionality (no explicit behavior change)
• document any changes to public APIs
• post benchmark scores
• manually test the debug page
• tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

/cc @asheemmamoowala @ryanbaumann @mollymerp @ansis

[mollymerp]

nice! thanks for tracking this down!

Peaking inside the VertexBuffer to test if it's "live" (e.g. this.paintVertexBuffer.buffer) -- it kind of feels like this should be handled by VertexBuffer somehow. If buffer is null, that means destroy() has already been called, and it needs to be re-recreated (I don't actually know of a pathway that would cause this to happen).

we could add a method on VertexBuffer that takes care of this check but I don't this its a big deal either.

We call updateData regardless of whether DYNAMIC_DRAW is enabled (it's cued off expression.isStateDependent). But I believe updateData is still fine if it ends up getting called on a STATIC_DRAW buffer (should be no worse than destroying and re-creating the buffer).

I think this should actually be better than destroying and recreating a new buffer 🎉 (per https://kripken.github.io/emscripten-site/docs/optimizing/Optimizing-WebGL.html)

[asheemmamoowala]

Duplication between CompositeExpressionBinder and SourceExpressionBinder

I was bothered by this too, but doesn't seem to be a good way to reduce this for now.

We call updateData regardless of whether DYNAMIC_DRAW is enabled (it's cued off expression.isStateDependent). But I believe updateData is still fine if it ends up getting called on a STATIC_DRAW buffer (should be no worse than destroying and re-creating the buffer).

In #5150, you added an assert to IndexBuffer#updateData to ensure it is using DYNAMIC_DRAW. Is that necessary here as well? As you say, it is not wrong to call bufferSubData on an array buffer created with STATIC_DRAW. Would it be useful though, for future changes to ensure that we are separating the static and dynamic buffer usage correctly?

[ChrisLoer]

Would it be useful though, for future changes to ensure that we are separating the static and dynamic buffer usage correctly?

🤔 I'm not sure, but an "assert" feels too strong to me here. It's not like it would represent a logical error, it would just be "a pathway we didn't expect" (admittedly those are pretty close concepts). But I can imagine cases where we might occasionally want to update a buffer but still expect it to be rare enough that STATIC_DRAW was more appropriate than DYNAMIC_DRAW...

[asheemmamoowala]

I can imagine cases where we might occasionally want to update a buffer but still expect it to be rare enough that STATIC_DRAW was more appropriate than DYNAMIC_DRAW

The difference between those buffer creation modes is driver dependent. Without profiling individual drivers it is not very useful to try an optimize the use of STATIC_DRAW vs DYNAMIC_DRAW especially given that we won't always know if the runtime APIs will actually be used to update buffers, and what each drivers optimization for this looks like.