Summary: | Make ElementAttributeData a variable-sized object to reduce memory use. | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||||||||||||||||
Component: | DOM | Assignee: | Andreas Kling <kling> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | cmarcelo, dglazkov, macpherson, menard, mifenton, ossy, rniwa, webkit.review.bot | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 88256, 91263 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Andreas Kling
2012-06-04 10:14:29 PDT
Created attachment 145604 [details]
Pøtch
Attachment 145604 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/ElementAttributeData.h:85: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/dom/ElementAttributeData.h:86: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 145604 [details] Pøtch Attachment 145604 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12905190 Comment on attachment 145604 [details] Pøtch View in context: https://bugs.webkit.org/attachment.cgi?id=145604&action=review r=me, nice! > Source/WebCore/dom/Element.cpp:684 > inline void Element::setAttributeInternal(size_t index, const QualifiedName& name, const AtomicString& value, EInUpdateStyleAttribute inUpdateStyleAttribute) > { > + mutableAttributeData(); This looks lonely. It would perhaps be nicer to use the returned value instead of accessing attributes directly. > Source/WebCore/dom/Element.cpp:2123 > +void Element::createMutableAttributeData() > +{ > + if (!m_attributeData) > + m_attributeData = ElementAttributeData::create(); It would be nicer if this was called ElementAttributeData::createMutable() (same for StylePropertySet::create() too) > Source/WebCore/dom/ElementAttributeData.cpp:54 > +ElementAttributeData::ElementAttributeData(const Vector<Attribute>& attributes) > + : m_isMutable(false) > + , m_arraySize(attributes.size()) > +{ > + for (unsigned i = 0; i < attributes.size(); ++i) > + new (&array()[i]) Attribute(attributes[i]); > +} Anders tells that we should be able to generalize this pattern into a template. Would be nice to do that at some point as we can probably find other uses. Committed r119421: <http://trac.webkit.org/changeset/119421> (In reply to comment #5) > Committed r119421: <http://trac.webkit.org/changeset/119421> Hey man, why did you commit it, when the Qt EWS bubble was red? Could you fix the build? Re-opened since this is blocked by 88256 Created attachment 151261 [details]
EWSxperiment
Attachment 151261 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/ElementAttributeData.h:83: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/dom/ElementAttributeData.h:85: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 151261 [details] EWSxperiment Attachment 151261 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13165333 Comment on attachment 151261 [details] EWSxperiment Attachment 151261 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13157875 Created attachment 151269 [details]
EWSxperiment 2
Attachment 151269 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/ElementAttributeData.h:83: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/dom/ElementAttributeData.h:85: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 151269 [details] EWSxperiment 2 Attachment 151269 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13174145 New failing tests: svg/css/svg-ellipse-render-crash.html Created attachment 151286 [details]
Archive of layout-test-results from gce-cr-linux-06
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 151711 [details]
EWSxperiment 3
Comment on attachment 151711 [details] EWSxperiment 3 View in context: https://bugs.webkit.org/attachment.cgi?id=151711&action=review r=me, glorious > Source/WebCore/dom/Element.cpp:244 > + // FIXME: This shouldn't have to make the ElementAttributeData mutable. > + const_cast<Element*>(this)->ensureUpdatedAttributeData(); Is there is really anything wrong with this? The FIXME might be an overkill (and point to over-optimization). Un-consting the function would remove that awkward const_cast. > Source/WebCore/dom/ElementAttributeData.cpp:69 > + , m_mutableAttributeVector(new Vector<Attribute, 4>) > +{ > + // This copy constructor should only be used by makeMutable() to go from immutable to mutable. > + ASSERT(!other.m_isMutable); > + for (unsigned i = 0; i < other.m_arraySize; ++i) > + m_mutableAttributeVector->append(other.array()[i]); > +} This could use reserverInitialCapacity() and uncheckedAppend(). Not sure how to calculate the optimal initial capacity. Maybe just 1 more than the current size? Committed r122450: <http://trac.webkit.org/changeset/122450> Re-opened since this is blocked by 91263 Created attachment 153807 [details]
Patch
Okay, let's give this another try. This time with liberal use of the "const" keyword to catch any incorrect ElementAttributeData usage at compile-time.
Comment on attachment 153807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153807&action=review > Source/WebCore/ChangeLog:155 > + Updated code to handle both immutable and mutable source objects. This could ...or could it? Comment on attachment 153807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153807&action=review > Source/WebCore/dom/Element.cpp:1398 > RefPtr<Attr> oldAttr = attrIfExists(attr->qualifiedName()); It appears that attrIfExists doesn't update the invalid attributes. > Source/WebCore/dom/ElementAttributeData.cpp:37 > + void* slot = WTF::fastMalloc(sizeof(ElementAttributeData) - sizeof(void*) + sizeof(Attribute) * attributes.size()); We don't have a fancy WTF macros to deal with alignments? > Source/WebCore/dom/ElementAttributeData.cpp:82 > + for (unsigned i = 0; i < m_arraySize; ++i) > + array()[i].~Attribute(); I think it's better to wrap this in the else clause. I would also assert that m_arraySize is 0 when isMutable(). > Source/WebCore/dom/ElementAttributeData.h:140 > + void* m_attributes; Why can't we use Attribute* here? (In reply to comment #22) > (From update of attachment 153807 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153807&action=review > > > Source/WebCore/dom/Element.cpp:1398 > > RefPtr<Attr> oldAttr = attrIfExists(attr->qualifiedName()); > > It appears that attrIfExists doesn't update the invalid attributes. Good catch. Fixing that and adding a test so it's covered in the future. > > Source/WebCore/dom/ElementAttributeData.cpp:37 > > + void* slot = WTF::fastMalloc(sizeof(ElementAttributeData) - sizeof(void*) + sizeof(Attribute) * attributes.size()); > > We don't have a fancy WTF macros to deal with alignments? Not sure what you're talking about. Subtracting sizeof(void*) is to get the base size of the object excluding the union. > > Source/WebCore/dom/ElementAttributeData.cpp:82 > > + for (unsigned i = 0; i < m_arraySize; ++i) > > + array()[i].~Attribute(); > > I think it's better to wrap this in the else clause. I would also assert that m_arraySize is 0 when isMutable(). Will do. > > Source/WebCore/dom/ElementAttributeData.h:140 > > + void* m_attributes; > > Why can't we use Attribute* here? We could use any type, at the end of the day we have to reinterpret_cast<Attribute*>(&m_attributes) anyway. It would look nicer if the compiler would let us do "Attribute m_attributes[0]" or something like that, but that doesn't fly in a union. :/ Created attachment 154165 [details]
Patch
Created attachment 154209 [details]
Patch
C'mon new EWS.
Dumping IRC log since rniwa pinged out. rniwa: kling: one more question about the patch rniwa: kling: what happens if CSSMutableStyle had been created for inline style rniwa: kling: when the attributes were immutable (i.e. stored in ElementAttributeData) rniwa: kling: and then attributes got mutable via setAttribute on the element (not via CSSOM wrapper for the inline style) rniwa: kling: doesn't the m_propertySet in the wrapper get stable in that case? kling: rniwa: the StylePropertySet pointer doesn't change when making a new mutable ElementAttributeData kling: rniwa: it inherits the old one from the immutable EAD kling: rniwa: or what do you mean by stable? kling: rniwa: if you look at ElementAttributeData(const ElementAttributeData& other), you can see that all members are cloned kling: rniwa: the source object is discarded after this constructor runs (the immutable EAD has now been replaced by a mutable EAD) kling: rniwa: (so there aren't multiple EADs pointing to the same m_propertySet) Comment on attachment 154209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154209&action=review > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:410 > + if (mutablePropertySet != m_propertySet) { When does this happen? > Source/WebCore/dom/ElementAttributeData.cpp:235 > ASSERT(index < length()); Missing ASSERT(isMutable()); here. > Source/WebCore/dom/StyledElement.cpp:237 > + bool changes = mutableAttributeData()->inlineStyle()->removeProperty(propertyID); We can't call ensureInlineStyle() here? Comment on attachment 154209 [details] Patch Attachment 154209 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13341133 New failing tests: fast/loader/loadInProgress.html fast/inline/positionedLifetime.html fast/loader/unload-form-post-about-blank.html Created attachment 154263 [details]
Archive of layout-test-results from gce-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
(In reply to comment #27) > (From update of attachment 154209 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154209&action=review > > > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:410 > > + if (mutablePropertySet != m_propertySet) { > > When does this happen? Never! D'oh! When InlineCSSStyleDeclaration is created, we always have a mutable StylePropertySet already. I'll replace this by an assertion that isMutable(). The code can be cleaned up further, I'll do that in a follow-up. > > Source/WebCore/dom/ElementAttributeData.cpp:235 > > ASSERT(index < length()); > > Missing ASSERT(isMutable()); here. Good idea. > > Source/WebCore/dom/StyledElement.cpp:237 > > + bool changes = mutableAttributeData()->inlineStyle()->removeProperty(propertyID); > > We can't call ensureInlineStyle() here? Sure we can! Created attachment 154353 [details]
Patch
Comment on attachment 154353 [details] Patch Clearing flags on attachment: 154353 Committed r123636: <http://trac.webkit.org/changeset/123636> All reviewed patches have been landed. Closing bug. |