Summary: | Create GraphicsContext3DState to aggregate state objects | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bruno Abinader (history only) <bruno.abinader> | ||||
Component: | Layout and Rendering | Assignee: | Bruno Abinader (history only) <bruno.abinader> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | dino, d-r, igor.oliveira, kbr, mifenton, mrobinson, noam, rwlbuis, tonikitoo, webkit.review.bot, yong.li.webkit | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 110883 | ||||||
Attachments: |
|
Description
Bruno Abinader (history only)
2013-02-25 16:03:27 PST
Created attachment 190145 [details]
Patch
Proposed patch
(In reply to comment #1) > Created an attachment (id=190145) [details] > Patch > > Proposed patch LGTM, but I will defer to No'am or Mrobinson a final word. Comment on attachment 190145 [details]
Patch
I'm not sure adding a new layer in an already byzantine class hierarchy is a good tradeoff for deleting six lines of code. I'm not going to block this patch, but I thought I'd mention that.
(In reply to comment #3) > (From update of attachment 190145 [details]) > I'm not sure adding a new layer in an already byzantine class hierarchy is a good tradeoff for deleting six lines of code. I'm not going to block this patch, but I thought I'd mention that. I agree - something about this seems like refactoring for the sake of refactoring. (In reply to comment #3) > (From update of attachment 190145 [details]) > I'm not sure adding a new layer in an already byzantine class hierarchy is a good tradeoff for deleting six lines of code. I'm not going to block this patch, but I thought I'd mention that. The GC3DState struct is also the baseline for an upcoming effort to add more state variables to avoid redundant state changes not covered by this patch (I'll create a bug related to this soon), so in fact the removal of the duplicated values is a plus, not the real goal. (In reply to comment #5) > The GC3DState struct is also the baseline for an upcoming effort to add more state variables to avoid redundant state changes not covered by this patch (I'll create a bug related to this soon), so in fact the removal of the duplicated values is a plus, not the real goal. Redundantly setting the bound texture, the active texture unit, or the FBO should be fast, unless you have some really poor drivers. (In reply to comment #6) > (In reply to comment #5) > > > The GC3DState struct is also the baseline for an upcoming effort to add more state variables to avoid redundant state changes not covered by this patch (I'll create a bug related to this soon), so in fact the removal of the duplicated values is a plus, not the real goal. > > Redundantly setting the bound texture, the active texture unit, or the FBO should be fast, unless you have some really poor drivers. These are not the target of bug 110883. I'll cover for now just the bottlenecks I've been detecting by profiling EFL's MiniBrowser under gDEBugger tool (so far: blend, stencil, scissor, depth mask-related functions). I've moved these variables to GC3DState for readibility, but I can update the patch just to create GC3DState struct if that's the case. s/readibility/readability/ :) I've uploaded the proposed patch on bug 110883 for your appreciation (depends on the proposed patch from this bug, as it creates the GC3DState struct). @Martin, @Kenneth: I've spoke w/ No'am today about this, he seems fine with the changes (this patch and the complimentary in bug 110883). I agree these changes gives minimum or no optimization on well-implemented drivers, but for others, this could reduce the amount of redundant state changes (speaking of bug 110883). Speaking of which, I've splitted the code in two for clarity, but if it makes more sense to aggregate both patches, please tell me so I can do so. Comment on attachment 190145 [details]
Patch
This refactoring looks good.
Comment on attachment 190145 [details] Patch Clearing flags on attachment: 190145 Committed r144378: <http://trac.webkit.org/changeset/144378> All reviewed patches have been landed. Closing bug. |