cc @jfirebaugh @tmcw @ansis @kkaefer @mourner

Reviewed the class, generally I think this is great. There's quite a bit of complexity introduced here but I guess we can't get away from it if we want generic data-driven styling with dynamically generated buffer layouts.

What I'd love to see is an example of a heavy buffer such as LineVertexBuffer and GlyphVertexBuffer rewritten using Buffer2, and some benchmarks/profiling. buffer.add(...) has a shitton of calls in the worker and takes a considerable amount of time, so the previous code was kept deliberately super-simple to make it really fast. Can we achieve comparable performance with a generic implementation?

I also think that to make this PR mergeable, we need to rewrite everything to use Buffer2 instead of Buffer and remove the old implementation, even if we know that this is eventually going away with the Bucket/Layer2 stuff. It seems relatively easy, and this way we won't have any code without use floating around, and will be able to catch any remaining issues.

This looks great!

I have the same question as @mourner about whether this is as fast as explicitly written versions.

Which methods are called in the worker? which are called in the main thread? If there is a clean split it might be good to separate Buffer2 into two parts.

What I'd love to see is an example of a heavy buffer such as LineVertexBuffer and GlyphVertexBuffer rewritten using Buffer2, and some benchmarks/profiling... Can we achieve comparable performance with a generic implementation?

I have done experiments with circle layers and found some performance degredation (ballpark ~15% slower). I would like to implement a more complex layer type, such as "line" or "glyph" and see how the performance scales but haven't gotten to that yet (more on this below)

Chances are we will have to choose between the increased flexibility of a more-generic-but-less-performant-infrastructure or a less-generic-but-more-performant-infrastructure.

My gut feeling is that there's nothing agregiously slow in the hot path (just a couple function calls and some loops) and we should be able to make this work.

Also, this refatoring DRYs up our buffer generation code. This will better position us to implement more complex performance optimzations in the future.

I also think that to make this PR mergeable, we need to rewrite everything to use Buffer2 instead of Buffer and remove the old implementation...

I started this project thinking I could create one PR that implemented Buffer2 and completely removed the old Buffer class. But it felt bad to sink so much time into refactoring the Bucket classes while planning to remove them in the very near future. So I started cramming the Layer2 class into this PR too and then things got way too complicated for a single PR.

My feeling is that allowing dead code in an isolated branch is a reasonable price to pay for a bunch of standalone ~500 line PRs instead of one XX000 line PR.

Which methods are called in the worker? which are called in the main thread? If there is a clean split it might be good to separate Buffer2 into two parts.

All methods are called in the worker (except get -- that's purely for debugging).

All methods except push, set, setAttribute, serialize are called in the main thread (that's most of them).

I do feel desire to split this class into two and have spent some time thinking about it. But I don't see any immediate practical benefit to doing so -- we'd end up with two small tightly-coupled classes instead of one nicely-sized class. Keeping the logic for understanding buffer layouts alongside the logic for writing to those buffers makes sense to me.

Chances are we will have to choose between the increased flexibility of a more-generic-but-less-performant-infrastructure or a less-generic-but-more-performant-infrastructure.

Sure. Something like a 15% slowdown sounds perfectly fine. But let's make sure on a heavy buffer.

But it felt bad to sink so much time into refactoring the Bucket classes while planning to remove them in the very near future.

Instead of refactoring buckets, could we just rewrite all *Buffer classes to extend Buffer2? That sounds like it could take way less time.

we'd end up with two small tightly-coupled classes instead of one nicely-sized class. Keeping the logic for understanding buffer layouts alongside the logic for writing to those buffers makes sense to me.

We could also have those two classes alongside each other in the same file (maybe one inheriting another). It could make the separation more clear — e.g. I'm not a fan of stuff like this (in the constructor here):

if (worker) {
  // huge chunk of code
} else {
  // something completely different
}

Instead of refactoring buckets, could we just rewrite all *Buffer classes to extend Buffer2? That sounds like it could take way less time.

That sounds like a good way to get this in the door. What do you think @jfirebaugh @ansis?

I'm not a fan of stuff like this...

To be fair, the fork in the constructor is splitting on whether we're unpacking a serialized instance or creating a new instance. I think that we should split this class into two if/when it gets unwieldy to share all code between the worker and main thread but it looks pretty clean right now imo. We might even want to mutate the buffer in the main thread in some cases.

Going to recut this PR... Hold on to your 🎩 👒