Remove code repetition in value.js #7358
Conversation
|
|
|
This looks great to me! I know this area is super performance sensitive and during the first implementation we had something more in this direction and it was reversed after profiling. The paint benchmark looks great but should we do some some more granular profiling? |
src/gl/value.js
Outdated
| return null; | ||
| } | ||
| _set(v: any) { | ||
| if (this.vao) { |
There was a problem hiding this comment.
Thinking out loud to document the change here...
Before, if we didn't have context.extVertexArrayObject, we would rely on code in context.js to never set this.dirty to true, so we'd never try to bind the object. Now, we don't depend on that code in context.js to keep us from attempting to bind, but it still prevents us from storing any value in this.current (although it also looks like all the calls to bindVertexArrayOES.set also check for the extension before running).
So looks like just one more layer of belt-and-suspenders protection against accidentally using a missing extension. 👍.
| setDefault(): void { this.set(this.default); } | ||
|
|
||
| set(v: ?WebGLTexture): void { | ||
| if (this.dirty || this.current !== v) { |
There was a problem hiding this comment.
Was there a reason that the dirty/inequality check order was reversed here before? I'm not actually sure what the equality operator for WebGLTexture does although it seems unlikely to make any difference that we can now call it more frequently (when the ColorAttachment is dirty).
There was a problem hiding this comment.
I think the base class set function does the dirty/inequality check before calling each child class' _set function.
There was a problem hiding this comment.
I think there's no difference — comparing boolean shouldn't be slower than comparing object references.
There was a problem hiding this comment.
ooh I missed the key word reversed – nvm!
|
It looks like the _set/_equals methods don't get inlined for some reason, so while there's a few percent slowdown in profiles which might not reflect on the paint benchmark, I'm not comfortable merging it. So I'll try to find out why V8 doesn't inline, and if it can't be helped, roll back halfway so that at least |
|
OK, I reintroduced explicit
|
Made a base class for value classes to remove code repetition and improve clarity.
Launch Checklist
write tests for all new functionalityalready covereddocument any changes to public APIs