Bug 77879 - Move style related functions from NamedNodeMap to ElementAttributeData
Summary: Move style related functions from NamedNodeMap to ElementAttributeData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on:
Blocks: 77952
  Show dependency treegraph
 
Reported: 2012-02-06 09:02 PST by Caio Marcelo de Oliveira Filho
Modified: 2012-02-07 04:09 PST (History)
3 users (show)

See Also:


Attachments
Patch (8.61 KB, patch)
2012-02-06 09:07 PST, Caio Marcelo de Oliveira Filho
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2012-02-06 09:02:29 PST
Move style related functions from NamedNodeMap to ElementAttributeData
Comment 1 Caio Marcelo de Oliveira Filho 2012-02-06 09:07:23 PST
Created attachment 125661 [details]
Patch
Comment 2 Ryosuke Niwa 2012-02-06 10:10:37 PST
Comment on attachment 125661 [details]
Patch

Excellent!
Comment 3 Caio Marcelo de Oliveira Filho 2012-02-06 10:37:48 PST
Committed r106819: <http://trac.webkit.org/changeset/106819>
Comment 4 Tim Horton 2012-02-06 13:32:31 PST
This change seems to significantly (~28% or so) regress performance of the ManInBlue HTML (http://themaninblue.com/experiment/AnimationBenchmark/html/) benchmark, among others. Looking at the change, though, I'm not sure how that's possible. Could you potentially take a look at this?
Comment 5 Ryosuke Niwa 2012-02-06 14:22:20 PST
(In reply to comment #4)
> This change seems to significantly (~28% or so) regress performance of the ManInBlue HTML (http://themaninblue.com/experiment/AnimationBenchmark/html/) benchmark, among others. Looking at the change, though, I'm not sure how that's possible. Could you potentially take a look at this?

That's surprising. The only reason I can think of this is happening if some functions in NamedNodeMap were benefiting from ensureInlineStyleDecl, destroyInlineStyleDecl, and ensureAttributeStyle being inlined. Could you try that and see what happens?
Comment 6 Ryosuke Niwa 2012-02-06 21:32:18 PST
The regression is probably caused by additional tertiary operators in inlineStyleDecl() and friends. We should probably do this refactoring when we move m_attributeData from NamedNodeMap to Element.
Comment 7 Ryosuke Niwa 2012-02-06 23:12:22 PST
king is right. This is caused by ensureInlineStyleDecl now calling ensureAttributeData, which updates attributes, instead of ensureInlineStyleDecl, which doesn't update attributes:

As a result, we're spending 2+% of time in StyledElement::style().
Comment 9 Tony Gentilcore 2012-02-07 04:03:56 PST
This appears to have regressed several of chromium's page cyclers and is a blocker for us.

Would you be opposed to rolling out this patch and working on the fix off of the tree?
Comment 10 Tony Gentilcore 2012-02-07 04:04:51 PST
Chromium bug: http://code.google.com/p/chromium/issues/detail?id=112854#c21
Comment 11 Tony Gentilcore 2012-02-07 04:09:25 PST
Actually, looks like rniwa has a proposed fix here: https://bugs.webkit.org/show_bug.cgi?id=77952