Bug 130226

Summary: [Mac] Avoid creating DOMCSSStyleDeclaration in WebHTMLConverter
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, darin, enrica, kling, koivisto, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 130227    
Attachments:
Description Flags
Extracted HTMLConverterCaches
kling: review+
A/B test result none

Ryosuke Niwa
Reported 2014-03-13 23:33:42 PDT
Right now, we spend a significant amount of time creating Objective-C wrappers for CSSStyleDeclaration and then looking up NSMutableDictionary.
Attachments
Extracted HTMLConverterCaches (26.20 KB, patch)
2014-03-13 23:37 PDT, Ryosuke Niwa
kling: review+
A/B test result (22.59 KB, text/html)
2014-03-13 23:47 PDT, Ryosuke Niwa
no flags
Ryosuke Niwa
Comment 1 2014-03-13 23:37:32 PDT
Created attachment 226655 [details] Extracted HTMLConverterCaches
Ryosuke Niwa
Comment 2 2014-03-13 23:47:38 PDT
Created attachment 226657 [details] A/B test result It's crazy. This simple C++ rewrite is a 20% improvement.
Andreas Kling
Comment 3 2014-03-14 00:36:39 PDT
Comment on attachment 226655 [details] Extracted HTMLConverterCaches View in context: https://bugs.webkit.org/attachment.cgi?id=226655&action=review r=me > Source/WebCore/platform/mac/HTMLConverter.mm:568 > + auto result = m_computedStyles.add(element, nullptr); > + if (result.isNewEntry) > + result.iterator->value = CSSComputedStyleDeclaration::create(element, true); > + CSSStyleDeclaration* declaration = result.iterator->value.get(); This would be more efficient using ComputedStyleExtractor directly. Then we wouldn't have to build a CSSComputedStyleDeclaration object at all. > Source/WebCore/platform/mac/HTMLConverter.mm:569 > + return declaration ? declaration->getPropertyCSSValue(propertyName) : 0; 0 -> nullptr > Source/WebCore/platform/mac/HTMLConverter.mm:577 > + auto result = m_inlineStyles.add(element, nullptr); > + if (result.isNewEntry) > + result.iterator->value = element->style(); > + CSSStyleDeclaration* declaration = result.iterator->value.get(); This code would be more efficient if it accessed the StyleProperties directly via Element::inlineStyle() instead of instantiating a CSSOM wrapper via style(). > Source/WebCore/platform/mac/HTMLConverter.mm:578 > + return declaration ? declaration->getPropertyCSSValue(propertyName) : 0; 0 -> nullptr > Source/WebCore/platform/mac/HTMLConverter.mm:785 > + Element* element = toElement(coreNode); I'd make this Element&. > Source/WebCore/platform/mac/HTMLConverter.mm:1005 > + Element* element = toElement(coreNode); Same here. > Source/WebCore/platform/mac/HTMLConverter.mm:2424 > + _caches = nullptr; Is this actually necessary?
Ryosuke Niwa
Comment 4 2014-03-14 01:04:24 PDT
(In reply to comment #3) > (From update of attachment 226655 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226655&action=review > > > Source/WebCore/platform/mac/HTMLConverter.mm:568 > > + auto result = m_computedStyles.add(element, nullptr); > > + if (result.isNewEntry) > > + result.iterator->value = CSSComputedStyleDeclaration::create(element, true); > > + CSSStyleDeclaration* declaration = result.iterator->value.get(); > > This would be more efficient using ComputedStyleExtractor directly. Then we wouldn't have to build a CSSComputedStyleDeclaration object at all. Can I store ComputedStyleExtractor in the hash map and use it later?
Andreas Kling
Comment 5 2014-03-14 10:17:52 PDT
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 226655 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=226655&action=review > > > > > Source/WebCore/platform/mac/HTMLConverter.mm:568 > > > + auto result = m_computedStyles.add(element, nullptr); > > > + if (result.isNewEntry) > > > + result.iterator->value = CSSComputedStyleDeclaration::create(element, true); > > > + CSSStyleDeclaration* declaration = result.iterator->value.get(); > > > > This would be more efficient using ComputedStyleExtractor directly. Then we wouldn't have to build a CSSComputedStyleDeclaration object at all. > > Can I store ComputedStyleExtractor in the hash map and use it later? Totally.
Ryosuke Niwa
Comment 6 2014-03-14 12:32:46 PDT
Note You need to log in before you can comment on or make changes to this bug.