Refactored Bucket class

[lucaswoj]

• made elementGroups always an object of multiple instances of ElementGroups
• removed createBucket module in favor of a simple Bucket.create method and a smarter constructor
• moved each attribute calculation into an individual function

[mourner]

Where did this come from?

[lucaswoj]

Oops! Just some innocuous-looking leftover debugging code. Will remove in the next commit.

[mourner]

We might have better luck with separate push calls for each separate value. There's was a severe performance regression on it in V8 that I reported https://code.google.com/p/v8/issues/detail?id=3883

[mourner]

Ah, nevermind, this is buffer push.

[lucaswoj]

We might have better luck with separate push calls for each separate value

I benchmarked this last week anyway and was surprised to see little performance improvement. Looks like trading array allocations for stack allocations is a wash.

[mourner]

This PR generally looks good, but I have the same concerns as in the previous PR — value functions and temporary array for each vertex.

The general issue here is that I don't yet see the big picture of how we'll use that later with disablable/updatable attributes, so I can't come up with a better way since it might interfere with your vision of data-driven style implementation. Could we cut out this specific change and add it in a different PR that actually makes use of the change (like a disabled attribute)?

[lucaswoj]

Could we cut out this specific change and add it in a different PR that actually makes use of the change (like a disabled attribute)?

I'm open to that. I'll back out those changes this morning.

[lucaswoj]

On further thought, I don't think this PR makes much sense with the removal of value functions (or some equivalent structure). I have an 😈 idea that might win us the performance gains we need while keeping the flexibility of this approach.

[lucaswoj]

Don't know if this is too 😈 but it actually results in somewhat simpler code. Interestingly, it does not seem to confer a big performance benefit.

url	95% time	average time	# samples
http://localhost:9000/evil.js	6184ms	3912ms	113
http://localhost:9000/master.js	3357ms	1978ms	124
http://localhost:9000/bucket3.js	10135ms	5683ms	90
http://localhost:9000/bucket3.js	8237ms	4858ms	96
http://localhost:9000/master.js	3069ms	1920ms	124
http://localhost:9000/evil.js	7369ms	4716ms	102

[lucaswoj]

Interesting clue: LineBucket::addCurrentVertex is by far the hottest code in master and bucket3. It spends 3661.9 ms executing in bucket3 (self time) but only 807.5 ms executing in master.

[lucaswoj]

Looks like eliminating 1 property lookup from Bucket.add*Vertex improved perf between 25% and 50% 😮. I guess I'll try to remove more of those.

[mourner]

@lucaswoj you might have bumped under 600 characters limit for V8 function inlining which could cause the perf increase.

[mourner]

I don't think this PR makes much sense with the removal of value functions (or some equivalent structure)

It doesn't make much sense with them as well because it's still not clear how to move this forward without a clear use for the separate attribute value calculations. Without the code actually making use of the change, it's not clear how I could change it to match previous performance, and obviously we can't merge something that makes tile loading 2-3 times slower without a good reason.

To unstall the PR, you'll have to either add the code that justifies the value functions change (e.g. some basic form of disablable attributes), or at least give a detailed description of how exactly the disabling will work with this code.

[lucaswoj]

With the latest commit, this branch is nearly as fast as master!

url	95% time	average time	# samples
http://localhost:9000/latest.js	3472ms	2122ms	121
http://localhost:9000/master.js	3729ms	2320ms	118
http://localhost:9000/latest.js	4342ms	2749ms	119
http://localhost:9000/master.js	3685ms	2138ms	122
http://localhost:9000/latest.js	4007ms	2521ms	116
http://localhost:9000/master.js	3151ms	2012ms	118

(who knew it would be so easy)

---

give a detailed description of how exactly the disabling will work with this code.

I want to decouple the 1:1 relationship between shader attributes and buffer attributes -- that the attributes of one buffer layout are exactly the attributes of one shader type.

For data-driven styling to work, we are going to want a single shader to use global attributes and potentially attributes from multiple buffers.

This PR makes it possible to calculate the values of attributes independently, making it easy & efficient to send attribute values to different places and recalculate them independently.

We don't use that capability yet but there is no "dead" code in this PR.

[lucaswoj]

Just checked the following browsers for perf regressions (and found none)

• IE 11 on Windows
• Firefox on Windows
• Safari on OSX
• Chrome on OSX
• Firefox on OSX

[lucaswoj]

@mourner with these performance improvements and my description of why we need to separate the logic used to calculate each attribute, are you willing to consider merging this into master?

There are still a few dependencies we need to actually leverage disable-able attributes (items 3, 4, 5, and 6 in #1626).

[lucaswoj]

Things I've tried to further close the performance gap, to no avail

• caching generated functions
• generating a function for add*Element

[mourner]

@lucaswoj yes, I think this is pretty close to being merged. But it would great for @jfirebaugh to review this as well — big important PRs like this need more than one pair of 👀

[jfirebaugh]

Is this used anywhere?

[lucaswoj]

Nope. Deleted in the next commit.

[jfirebaugh]

I'm not clear on why we're introducing knowledge of shaders into this code. There's a loop in Bucket#resetBuffers that seems to be attempting to "deduplicate" buffers that are shared between multiple shaders. But this doesn't work unless the shaders specify the exact same set of attributes, elementBufferComponents, etc for the shared buffers. So why don't we just have Bucket subclasses specify their buffer formats, without bringing the concept of shaders into the mix?

[lucaswoj]

I'm not clear on why we're introducing knowledge of shaders into this code.

We are moving towards a paradigm in which a shader's attributes may span several Buffers. The Bucket class is given knowledge about the set of attributes required by a particular shader and then given latitude to fulfill those requirements in whatever manner makes the most sense, given the layer's style.

There's a loop in Bucket#resetBuffers that seems to be attempting to "deduplicate" buffers that are shared between multiple shaders. But this doesn't work unless the shaders specify the exact same set of attributes, elementBufferComponents, etc for the shared buffers.

Yes, there is an assumption that we don't introduce conflicting shader definitions. This problem will go away when we start supporting "disable-able attributes" and Buffers are assigned unique names based on the attributes that they include.

I will add an assertion that each shader is only defined once.

So why don't we just have Bucket subclasses specify their buffer formats, without bringing the concept of shaders into the mix?

The interface of the shader is fixed. The way we fulfill that interface will be decided by the Bucket class at runtime. We could call it something other than "shader" but fundamentally the Bucket class' job is to fulfill the shader's interface.

[jfirebaugh]

Okay, let me know if the following understanding is correct:

The current implementation statically groups attributes into buffers based on their use in shaders. The influence of shaders on the existing code is thus implicit. This PR makes the influence explicit. However, we want to be moving towards a model where the grouping of attributes into buckets is dynamically determined.

If that's correct, I would argue that making "shader" into an explicit domain concept in this part of the code is actually counterproductive to your goal. It sounds like a better domain model would be "attributes" and "attribute groups". For now, the attribute groups would be statically defined and (implicitly) correspond to shaders, but we'd have a domain model that wouldn't need to be reworked again for dynamic attribute grouping.

[lucaswoj]

Even when we have dynamic attribute grouping, we still need knowledge about the shader's attributes. The union of all dynamic attribute groups must equal the shader's attributes.

[lucaswoj]

So, for example, the CircleBucket class will have a static property containing metadata bout the shader's attributes and an instance of CircleBucket will decide how to design some dynamic attribute groups that, in aggregate, satisfy the shader's attributes.

[lucaswoj]

Also worth noting: my long-term vision is to merge this class, the StyleLayer subclass, and parts of the Painter class so that this is the single source of knowledge about the shader.

[jfirebaugh]

Let me try to approach this from a more concrete angle. We definitely envision a scenario where a single shader pulls data from multiple buffers. Do we envision a scenario where a single buffer supplies data to multiple shaders?

• If so, then "buffer" or "attribute group" should be a top-level domain object. Shaders could still be a domain object, but they should refer to attributes, not define them themselves. Basically, within the domain model it should be impossible to express an invalid configuration, e.g. where multiple shaders reference the same attributes, but supply conflicting metadata about them.
• If not, then we should remove the deduplication from Bucket#resetBuffers, and ensure that buffer names are unique.

[lucaswoj]

Do we envision a scenario where a single buffer supplies data to multiple shaders?

I believe the answer to this question is "no." If the shaders are different, the feature geometries are different and there is no meaningful way to share buffers.

If not, then we should remove the deduplication from Bucket#resetBuffers, and ensure that buffer names are unique.

I think we still want to deduplicate these buffers. If there are 10 line layers, we should only create one lineVertex buffer.

Alternately, we could have another entity responsible for creating all the buffers up front. This is not desirable once we start introducing dynamic attribute grouping, however.

[jfirebaugh]

👍

The loop I'm referring to is this one, and the deduplication is the conditions like !buffers[vertexBufferName]. It looks like your most recent commit ensures that these conditions are always true, so let's remove them.

[lucaswoj]

!buffers[vertexBufferName] would still be false if another instance of LineBucket in the same WorkerTile has already created the Buffer.

[jfirebaugh]

Ah, I see. Does that possibility exist only because we haven't fixed #1585 yet?

[lucaswoj]

Yes, #1585 would make this check unnecessary.

[jfirebaugh]

Great. Sorry for jumping to conclusions about why that was there.

🚢!

[lucaswoj]

No worries John! I really appreciate your thoroughness. 

I think I'm seeing a small perf regression in the latest commit -- maybe because of inlining limits? I'm going to give it a look on Monday before pulling the trigger.

On Fri, Oct 30, 2015 at 3:47 PM, John Firebaugh notifications@github.com
wrote:

Great. Sorry for jumping to conclusions about why that was there.

🚢!

Reply to this email directly or view it on GitHub:
#1645 (comment)

[lucaswoj]