Refactor Bucket class

[lucaswoj]

ref #1932
fixes #2004
fixes #1585

• transfer Buckets from the worker to the main thread
• inline ElementGroups into Bucket
• move Buffer#EXTENT to Bucket#EXTENT or elsewhere
• rename builders to buffers in buffer.test.js
• split up buffers Split up buffers #1585

[lucaswoj]

👀 @mourner @ansis @jfirebaugh?

[ansis]

We used to have Buckets in the main thread. Then #941 moved way from that:

The concept of a "bucket" is now something that only the workers need to know
about. The main thread does not create buckets, it merely stores the buffers
and elementGroups on the tile when they are received from the worker.

What are the reasons for switching back to having full Buckets in the main thread?

[lucaswoj]

What are the reasons for switching back to having full Buckets in the main thread?

The Bucket class does a couple of things, some of which are necessary on the main thread (knowing which attributes are disabled, knowing how to calculate attribute values) and some of which are not necessary on the main thread (knowing how to iterate over features to build buffers).

We should factor the Bucket class into several separate classes to isolate these distinct pieces of functionality. I think we can afford to do that refactor later.

[ansis]

We should factor the Bucket class into several separate classes to isolate these distinct pieces of functionality. I think we can afford to do that refactor later.

Sounds ok to me

I reviewed the changes and it looks good but we should get a second set of 👀 on this before merging.

[jfirebaugh]

We should factor the Bucket class into several separate classes to isolate these distinct pieces of functionality. I think we can afford to do that refactor later.

I probably won't have a chance to do a deep review here, but this sounds fine to me.

[lucaswoj]

@mourner Would you care to be @ansis' second set of eyes on this one?

[ansis]

Would you care to be @ansis' second set of eyes on this one?

That was mostly about the high level changes which @jfirebaugh already commented on. More review is always good though

• how does 7c41264 impact memory usage? Does it create a couple new 8KB buffers for each bucket even if the bucket has few or no features?
• does 7c41264 impact tile parsing time? I'd guess no, but it could be good to check.

[lucaswoj]

how does 7c41264 impact memory usage? Does it create a couple new 8KB buffers for each bucket even if the bucket has few or no features?

Yes, it does create 8KB buffers for each bucket. My understanding is that we take exactly this approach in mapbox-gl-native. Are there any other optimizations there?

does 7c41264 impact tile parsing time? I'd guess no, but it could be good to check.

I just ran the buffer benchmark and found a ~40% slowdown. We should investigate and try to ameliorate before merging.

[lucaswoj]

Creating and transferring more buffers is more expensive. By tweaking some parameters, I was able to get the buffer benchmark time to within 15% of master.

Another potential optimization is to use three[1] instances of window.ArrayBuffer as storage for many instances of mapboxgl.Buffer. This would be a medium-sized lift that could go into another PR.

[1] one vertex buffer, one element buffer, and one secondElement buffer

[lucaswoj]

I should note that, after doing parameter tuning, I came to an unintuitive optimal value for Buffer.CAPACITY_RESIZE_MULTIPLE of 5. Does this strike anyone as a red flag? @mourner @ansis @jfirebaugh?

[ansis]

Another potential optimization is to use three[1] instances of window.ArrayBuffer as storage for many instances of mapboxgl.Buffer.

Sounds good to me. In general, I'd prefer if optimizations to avoid regressions are done before merging into master so that master never regresses.

I should note that, after doing parameter tuning, I came to an unintuitive optimal value for Buffer.CAPACITY_RESIZE_MULTIPLE of 5. Does this strike anyone as a red flag?

What did you optimize for? allocation performance or memory usage? That doesn't seem surprising if all that matters is the allocation performance.

[lucaswoj]

Sounds good to me. In general, I'd prefer if optimizations to avoid regressions are done before merging into master so that master never regresses.

I prefer that too, especially given the recent perf regressions.

However, the requirement that PRs do not regress any performance metric by any amount is pretty stiff, especially in the context of these kinds of refactors, which are necessary to ship data-driven styling.

I will continue working on optimizing this PR today. As always, I appreciate extra eyes and thoughts on places where we could improve perf here.

What did you optimize for? allocation performance or memory usage? That doesn't seem surprising if all that matters is the allocation performance.

I am optimizing for the "buffer benchmark" the source of which is available here.

I reckon we can get optimal memory usage with this allocation strategy by "shrinkwrapping" the ArrayBuffers after they are populated

arrayBuffer.slice(0, this.length * this.itemSize)

[lucaswoj]

@ansis I added an ArrayBuffer size optimization in 80c2acb which should make memory usage lower than it was before this refactor and improve overall performance.

[lucaswoj]

On second thought, we could call this Buffer#trim

[ansis]

@lucaswoj the buffer optimizations look great. Definitely fast enough for now! thanks

It looks like transfers back to the main thread are still slower:

bucket-2004:

parsed tile transfer (ms): 10
parsed tile transfer (ms): 16
parsed tile transfer (ms): 16
parsed tile transfer (ms): 23
parsed tile transfer (ms): 16
parsed tile transfer (ms): 15

master:

parsed tile transfer (ms): 2
parsed tile transfer (ms): 2
parsed tile transfer (ms): 3
parsed tile transfer (ms): 3
parsed tile transfer (ms): 2
parsed tile transfer (ms): 2

those times were produced by https://gist.github.com/ansis/572fb5caf1baff2452f7
(click anywhere on the map to reparse all tiles)

It looks like this is caused by:

• sending a serialized layer with each bucket
  561c9b4#diff-c1d5cefb566acef357121481a5a8e27eR173
• having many small buffers

[lucaswoj]

With 5cf4b50, transfer times in the benchmark posted above are between 6 and 12 ms

[ansis]

@lucaswoj Looks ready to me!