| Summary: | Switch isQuirkValue() virtual function to inline one. | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tamas Czene <tczene> | ||||||||||||||||||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | ap, davidbarr, dglazkov, eric, hyatt, jamesr, koivisto, loki, macpherson, webkit.review.bot | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||
| Bug Blocks: | 64262 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Tamas Czene
2011-07-20 06:05:49 PDT
Created attachment 101455 [details]
Switch isQuirkValue virtual function to inline one.
Comment on attachment 101455 [details] Switch isQuirkValue virtual function to inline one. Attachment 101455 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9200248 New failing tests: css1/basic/comments.html css1/basic/grouping.html css1/box_properties/border_bottom_width.html css1/box_properties/border_bottom_width_inline.html css1/box_properties/border_left_width.html css1/basic/inheritance.html css1/box_properties/border_color_inline.html css1/box_properties/border_inline.html css1/basic/class_as_selector.html css1/box_properties/border.html css1/box_properties/border_right.html css1/box_properties/border_color.html css1/box_properties/border_left_width_inline.html css1/basic/id_as_selector.html css1/box_properties/border_left.html css1/box_properties/border_bottom_inline.html css1/basic/containment.html css1/box_properties/border_bottom.html css1/box_properties/border_left_inline.html css1/basic/contextual_selectors.html Created attachment 101461 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
See my comments in https://bugs.webkit.org/show_bug.cgi?id=64262. Comment on attachment 101455 [details]
Switch isQuirkValue virtual function to inline one.
Um. This increases the size of all CSSPrimativeValues. I suspect this could cause a large memory regression?
(In reply to comment #5) > (From update of attachment 101455 [details]) > Um. This increases the size of all CSSPrimativeValues. I suspect this could cause a large memory regression? I believe an alternative approach might hit both targets, investigating. Inreasing the size of CSSPrimativeValue is definitely not acceptable. The performance claims are obviously bs. This is WONTFIX. Another way of saying what Antti said: A change like this would need to provide evidence (results from a trusted, repeatable benchmark) that this change is a perf improvement. FWIW, I've also noticed that the virtual dispatch on CSSValue shows up a lot in profiling. I wouldn't be surprised if there were some performance gains to be found here by eliminating the virtual call, but as Eric points out, there is a tradeoff with the size on these objects because there tend to be a lot of them. (In reply to comment #10) > FWIW, I've also noticed that the virtual dispatch on CSSValue shows up a lot in profiling. I wouldn't be surprised if there were some performance gains to be found here by eliminating the virtual call, but as Eric points out, there is a tradeoff with the size on these objects because there tend to be a lot of them. I believe there are some wasted bits that could be put to good use for a perf enhancement without increasing the memory overhead. Please give a chance to upload an RFC patch. I'm all for eliminating virtual functions. This particular bug just didn't start very well (wild, unsubstantiated performance claims, patch that doesn't actually remove a virtual function and adds a member variable to hold value "false"). Better continue in a new bug. Hmm, I was wrong, it does actually eliminate the virtual call. The patch is just buggy. I think this can be recovered. Comment on attachment 101455 [details] Switch isQuirkValue virtual function to inline one. View in context: https://bugs.webkit.org/attachment.cgi?id=101455&action=review > Source/WebCore/css/CSSPrimitiveValue.h:27 > +#include <wtf/AlwaysInline.h> Not needed. > Source/WebCore/css/CSSPrimitiveValue.h:196 > - virtual bool isQuirkValue() { return false; } > + ALWAYS_INLINE bool isQuirkValue() { return m_isQuirkValue; } Remove ALWAYS_INLINE. This will almost surely inline without it. > Source/WebCore/css/CSSPrimitiveValue.h:206 > > + bool m_isQuirkValue; Move this to this bitfield: signed m_type : 31; mutable unsigned m_hasCachedCSSText : 1; Take one bit out from m_type. Make all member variables protected instead of private. > Source/WebCore/css/CSSQuirkPrimitiveValue.h:44 > { > + m_isQuirkValue = false; > } Should be set to true here. Created attachment 110958 [details]
Patch
Created attachment 110959 [details]
Patch
Comment on attachment 110959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110959&action=review > Source/WebCore/ChangeLog:9 > +2011-10-13 David Barr <davidbarr@chromium.org> > + > + Inline CSSPrimitiveValue::isQuirkValue() as non-virtual function > + https://bugs.webkit.org/show_bug.cgi?id=64865 > + > + Reviewed by NOBODY (OOPS!). > + > + This is expected to produce a small performance progression. > + Additional memory overhead is avoided by using spare bits. David, may I ask you to mention Tamas Czene here? Basically it was his idea, and the previous patch was done by him. Comment on attachment 110959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110959&action=review > Source/WebCore/css/CSSPrimitiveValue.h:249 > + unsigned m_isQuirkValue : 1; We should expose a protected inline setter for m_isQuirkValue instead of exposing the data member itself. Comment on attachment 110959 [details]
Patch
This idea is fine, but there is no reason to keep an entire class, CSSQuirkPrimitiveValue, any more given that the class has no contents. Instead we just need a new create function in CSSPrimitiveValue that creates a quirk value.
Doing it that way will remove an entire unnecessary vtable, make the code cleaner and easier to understand, and also remove the need to add new protected members.
I’m going to say review+ but I’d much prefer this be redone to eliminate the class.
(In reply to comment #19) > Doing it that way will remove an entire unnecessary vtable, make the code cleaner and easier to understand, and also remove the need to add new protected members. Unfortunately it will take much more than that to remove vtable from CSSValues. CSSPrimitiveValue is not the base class. > I’m going to say review+ but I’d much prefer this be redone to eliminate the class. Yeah, definitely. I didn't notice that there was nothing else there. Created attachment 111125 [details]
Patch
Comment on attachment 111125 [details]
Patch
As suggested, updated patch to remove now trivial class CSSQuirkPrimitiveValue and due credit given to Tamas Czene.
Comment on attachment 111125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111125&action=review Sorry for the noise, I noticed some nits just after uploading the patch. > Source/WebCore/WebCore.gypi:-2383 > - 'css/CSSQuirkPrimitiveValue.h', As a developer new to WebKit, I'm not familiar with the process of propagating this change. FWIW, it doesn't appear to break the build. > Source/WebCore/css/CSSQuirkPrimitiveValue.h:-31 > -// This value is used to handle quirky margins in reflow roots (body, td, and th) like WinIE. > -// The basic idea is that a stylesheet can use the value __qem (for quirky em) instead of em. > -// When the quirky value is used, if you're in quirks mode, the margin will collapse away > -// inside a table cell. I think this comment should have followed CSSQuirkPrimitiveValue::create() when it became CSSPrimitiveValue::createQuirk(). Comment on attachment 111125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111125&action=review Not setting commit-queue+ because of David’s comment, and my related one. >> Source/WebCore/css/CSSQuirkPrimitiveValue.h:-31 >> -// inside a table cell. > > I think this comment should have followed CSSQuirkPrimitiveValue::create() when it became CSSPrimitiveValue::createQuirk(). I agree. I also would suggest naming the new function CSSPrimitiveValue::createAllowingMarginQuirk(). Created attachment 111150 [details]
Patch
Comment on attachment 111150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111150&action=review > Source/WebCore/css/CSSPrimitiveValue.h:140 > + /* > + * This value is used to handle quirky margins in reflow roots (body, td, and th) like WinIE. > + * The basic idea is that a stylesheet can use the value __qem (for quirky em) instead of em. > + * When the quirky value is used, if you're in quirks mode, the margin will collapse away > + * inside a table cell. > + */ We use "//" comments, not "/*" comments. Not sure why you changed this one. > Source/WebCore/css/CSSPrimitiveValue.h:143 > + CSSPrimitiveValue *ptr = new CSSPrimitiveValue(value, type); The "*" goes next to the type, not the local variable name. Instead of ptr this local variable should be named value. Comment on attachment 111150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111150&action=review >> Source/WebCore/css/CSSPrimitiveValue.h:140 >> + */ > > We use "//" comments, not "/*" comments. Not sure why you changed this one. I was matching style with the nearest comment block in the destination file, which just happened to be the only incorrectly styled comment in the whole file. Will fix. >> Source/WebCore/css/CSSPrimitiveValue.h:143 >> + CSSPrimitiveValue *ptr = new CSSPrimitiveValue(value, type); > > The "*" goes next to the type, not the local variable name. > > Instead of ptr this local variable should be named value. Given that the parameter is also called value, I'll take the liberty of calling it quirkValue. This has the advantage that the middle line reads quite clearly: quirkValue->m_isQuirkValue = true; Created attachment 111152 [details]
Patch
Comment on attachment 111152 [details]
Patch
All nits addressed, requesting commit-queue.
Comment on attachment 111152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111152&action=review Seems OK. > Source/WebCore/css/CSSPrimitiveValue.h:257 > - signed m_type : 31; > + signed m_type : 30; Why is m_type bigger than it needs to be? (In reply to comment #30) > (From update of attachment 111152 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111152&action=review > > Seems OK. > > > Source/WebCore/css/CSSPrimitiveValue.h:257 > > - signed m_type : 31; > > + signed m_type : 30; > > Why is m_type bigger than it needs to be? It takes values from enum CSSPrimitiveValue::UnitType, the maximum of which is presently CSS_COUNTER_NAME = 109, so I think it only needs 7 unsigned bits. However it began as an int even though it's always implicitly cast to short by its accessors. One bit was borrowed for m_cachedCSSText in r59386 to resolve a substantially larger memory regression but the extra bits were left assigned to m_type. It seems Sam Weinig and I were both timid when shrinking it. Principal of least change perhaps? Comment on attachment 111152 [details]
Patch
Its fine to not shink it, but we should explain exacxtly what you just did. Where it gets its size from, and how large it currently would need to be. That way the next reviewer won't have any question when we lop another bit off it, since the comment right above will explain that it only uses 7 of the 31 bits and they can verify that by searching for the right enum.
Anyway, the change is fine. ADding a comment to explain woudl be gravy. Did we ever validate the profiling? Did we ever test this with the PLT or with PerformanceTests/Parser/html5-full-render.html? It seems at least one of those should show this change. Created attachment 111184 [details]
Patch
Comment on attachment 111184 [details]
Patch
Added comment above m_type declaration for future reference.
PerformanceTests/Parser/html5-full-render.html with 20 runs: ToT: Testing 6092696 byte document in 13 500000 byte chunks. Running 20 times Ignoring warm-up run (43271) 42328 42366 42265 42350 42285 42353 42270 42247 42309 42647 42622 42305 42305 42283 42286 42223 42162 42278 42375 42391 avg 42332.5 median 42305 stdev 113.7002638519366 min 42162 max 42647 With patch: Testing 6092696 byte document in 13 500000 byte chunks. Running 20 times Ignoring warm-up run (42394) 42100 42123 42166 42121 42017 42113 41883 41853 42158 41996 42160 42056 42209 42098 41933 42182 41880 41806 41981 42063 avg 42044.9 median 42080.5 stdev 117.5461186088252 min 41806 max 42209 That gives a progression of 2.45 stdevs or 0.7% Comment on attachment 111184 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111184&action=review > Source/WebCore/css/CSSPrimitiveValue.h:257 > + // This holds enum UnitType, so only 7 unsigned bits are necessary. I would recommend adding a UnitTypeBits constant defined as 7, remove this comment, and use the constant here. Also it would be good to add a COMPILE_ASSERT to the constructor of PrimitiveValue that asserts that its size never exceeds what you expect. Performance numbers look really good, and IMHO getting rid of the quirk value class is good too. Created attachment 111192 [details]
Patch
Comment on attachment 111192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111192&action=review > Source/WebCore/css/CSSPrimitiveValue.h:145 > + } Also, I'd delete this comment + code and and just add a boolean parameter that defaults to false to create(double, UnitTypes). Comment on attachment 111192 [details] Patch Attachment 111192 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10078747 Created attachment 111193 [details]
Patch
Comment on attachment 111193 [details]
Patch
Leaving m_type signed and 8 bits to work around 'sign mismatch in comparison' errors. While I agree that the semantics of constructing CSSPrimitiveValues for quirky ems could be improved, I think it is beyond the scope of this change.
Thank you David. Comment on attachment 111193 [details] Patch Clearing flags on attachment: 111193 Committed r97583: <http://trac.webkit.org/changeset/97583> All reviewed patches have been landed. Closing bug. |