Bug 54707

Created attachment 82891 [details] Patch

First step towards https://bugs.webkit.org/show_bug.cgi?id=46592 - see existing discussion on that bug.

*** Bug 54706 has been marked as a duplicate of this bug. ***

+// static Length convertToLength(CSSPrimitiveValue*, RenderStyle*, RenderStyle* rootStyle, double multiplier = 1, bool *ok = 0); +// static void applyCounterList(RenderStyle*, CSSValueList*, bool isReset); Please don't land commented out code.

Created attachment 83108 [details] Patch

Attachment 83108 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7935799

Attachment 83108 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7940518

Attachment 83108 [details] did not build on win: Build output: http://queues.webkit.org/results/7938670

Created attachment 83112 [details] Patch

Attachment 83112 [details] did not build on win: Build output: http://queues.webkit.org/results/7940541

Created attachment 83263 [details] Patch

Attachment 83263 [details] did not build on win: Build output: http://queues.webkit.org/results/7942193

Created attachment 83271 [details] Patch

Comment on attachment 83271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83271&action=review > Source/WebCore/css/CSSStyleApplyProperty.cpp:48 > + : m_prop(prop) I would have written out m_prop as m_property. > Source/WebCore/css/CSSStyleApplyProperty.cpp:54 > + virtual void inherit(CSSStyleSelector *instance) const = 0; arg names aren't useful here and should be removed. > Source/WebCore/css/CSSStyleApplyProperty.cpp:67 > + virtual void inherit(CSSStyleSelector *instance) const {UNUSED_PARAM(instance);} Better to not name the instance parameter at all. Also the * is in the wrong palce. > Source/WebCore/css/CSSStyleApplyProperty.cpp:134 > + if (value->isPrimitiveValue()) { I would have used early return. > Source/WebCore/css/CSSStyleApplyProperty.cpp:184 > + m_propertyMap[PROPERTY_INDEX(CSSPropertyDirection)] = m_invalid; This should just be a helper function. markInvalid(CSSPropertyName); Or you could put them in a static array and use a for loop. your choice.

I don't know why this passed the style bot. check-webkit-style should have warned about the misplaced * at least.

Created attachment 83735 [details] Patch

All Eric's comments addressed except for early return preference.

Comment on attachment 83735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83735&action=review > Source/WebCore/css/CSSStyleApplyProperty.cpp:182 > + m_propertyMap[PROPERTY_INDEX(CSSPropertyColor)] = new ApplyPropertyColor(CSSPropertyColor, &RenderStyle::color, &RenderStyle::setColor, RenderStyle::initialColor); This also looks like it wants to be a helper function, no?

Comment on attachment 83735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83735&action=review > Source/WebCore/css/CSSStyleApplyProperty.cpp:210 > +void CSSStyleApplyProperty::initial(int property, CSSStyleSelector* instance) Can't property be type specific (an enum) instead of int? Or do we not currently use an enum for these? > Source/WebCore/css/CSSStyleApplyProperty.cpp:213 > + ASSERT_VALID_PROPERTY(property); > + m_propertyMap[PROPERTY_INDEX(property)]->initial(instance); Seems this lookup is repeated. The ASSERT and PROPERTY_INDEX call, etc. could all be rolled into a lookup: propertyValue(property)->initial(instance); would then be all this needed to look like.

Comment on attachment 83735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83735&action=review > Source/WebCore/ChangeLog:9 > + > + No new tests. (OOPS!) > + You should talk more about why you're doing this change in the ChangeLog. What's the benifit? Is there a perf win here? Are we just making CSSStyleSelector more readable? You noted over IRC that this is only the first part of the changes. You should mention that in the ChangeLog. You should also replace the "no new tests" boiler plate with something like "This is already covered by many LayoutTests." or similar. > Source/WebCore/css/CSSStyleApplyProperty.cpp:58 > + int property() const { return m_property; } I know I mentioned this elsewhere, but it would be great if this could be an enum instead of an int. > Source/WebCore/css/CSSStyleApplyProperty.cpp:85 > + (instance->style()->*m_setter)((instance->parentStyle()->*m_getter)()); I'm not sure I understand why we're calling the function pointers directly here. Why wouldn't these be wrapped as functions on CSSStyleSelector. So we could just call something like: instance->styleSetter(instance->parentStyleGetter())? I'm not sure why these are function pointers. Maybe I'm just confused. :) Maybe this needs more explanation in the ChangeLog. :)

Comment on attachment 83735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83735&action=review > Source/WebCore/css/CSSStyleApplyProperty.h:55 > + void value(int property, CSSStyleSelector*, CSSValue *); * goes to the left. :) > Source/WebCore/css/CSSStyleApplyProperty.h:61 > + ApplyPropertyBase* m_propertyMap[numCSSProperties]; I"m confused why this is per-instance?

Comment on attachment 83735 [details] Patch r- for the numerous nits above.

Attachment 83735 [details] did not build on qt: Build output: http://queues.webkit.org/results/7986608

Created attachment 83762 [details] Patch

Attachment 83762 [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/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 83763 [details] Patch

Created attachment 84983 [details] Patch

Performance numbers: Loading wsj.com from a locally cached version @ 20uS sample rate using shark on OSX. JS and flash were disabled during testing to improve repeatability. These are counts of the samples taken in CSSStyleSelector::applyProperty(). Unmodified (r80210): self 431 total 431 of 58264 self 418 total 418 of 68394 self 398 total 398 of 70524 self 382 total 382 of 73188 self 409 total 409 of 71378 avg: 407.6 Modified (r80210) without inlining: self 401 total 401 of 68573 self 432 total 432 of 71032 self 453 total 453 of 71483 self 427 total 427 of 73488 self 421 total 421 of 68354 avg: 426.8 Modified (r80210) with inlining (in last patch): self 378 total 378 of 70430 self 405 total 405 of 71395 self 397 total 397 of 68309 self 395 total 395 of 68409 self 363 total 363 of 69917 avg: 387.6

Attachment 84983 [details] did not build on win: Build output: http://queues.webkit.org/results/8107424

(In reply to comment #28) > Performance numbers: > > Loading wsj.com from a locally cached version @ 20uS sample rate using shark on OSX. > JS and flash were disabled during testing to improve repeatability. > These are counts of the samples taken in CSSStyleSelector::applyProperty(). Am I reading correctly that your patch makes applyProperty hotter? What about total time running the PLT or laying out wsj.com?

(In reply to comment #30) > (In reply to comment #28) > > Performance numbers: > > > > Loading wsj.com from a locally cached version @ 20uS sample rate using shark on OSX. > > JS and flash were disabled during testing to improve repeatability. > > These are counts of the samples taken in CSSStyleSelector::applyProperty(). > > Am I reading correctly that your patch makes applyProperty hotter? What about total time running the PLT or laying out wsj.com? There are two sets of results there - modified without inlining was spending more time in applyProperty (ie slower), then I fixed the inlining issues by moving some code into the header, and now it is spending less time in applyProperty (ie. faster). The last set of results correspond to the last patch uploaded. Let me know if I can make this clearer, but the bottom line is that we now spend slightly less time in applyProperty and its children, for a net speed-up. I expect this will improve further as more properties are converted over.

(In reply to comment #31) > Let me know if I can make this clearer, but the bottom line is that we now spend slightly less time in applyProperty and its children, for a net speed-up. I expect this will improve further as more properties are converted over. OK. Although I believe you, numbers which encompass an entire layout would be more convincing (to me) than sample counts inside those functions. It's possible (although unlikely) that reducing the samples inside those functions might still have an increase in total time spent for a style-recalc or layout. Thanks again for working on this. I'm happy to take another look for review this evening (if someone doesn't beat me to it).

Attachment 84983 [details] did not build on win: Build output: http://queues.webkit.org/results/8101625

(In reply to comment #32) > (In reply to comment #31) > > Let me know if I can make this clearer, but the bottom line is that we now spend slightly less time in applyProperty and its children, for a net speed-up. I expect this will improve further as more properties are converted over. > > OK. Although I believe you, numbers which encompass an entire layout would be more convincing (to me) than sample counts inside those functions. It's possible (although unlikely) that reducing the samples inside those functions might still have an increase in total time spent for a style-recalc or layout. Uh, embarrassingly it looks like I posted numbers which are the time inside applyProperty, but not including its children. I blame shark's intuitive UI. Let me run it again and get back to you. Or I could just attach some profiles to the bug and everyone can look at whatever interests them.

(In reply to comment #34) > (In reply to comment #32) > > (In reply to comment #31) > > > Let me know if I can make this clearer, but the bottom line is that we now spend slightly less time in applyProperty and its children, for a net speed-up. I expect this will improve further as more properties are converted over. > > > > OK. Although I believe you, numbers which encompass an entire layout would be more convincing (to me) than sample counts inside those functions. It's possible (although unlikely) that reducing the samples inside those functions might still have an increase in total time spent for a style-recalc or layout. > > Uh, embarrassingly it looks like I posted numbers which are the time inside applyProperty, but not including its children. I blame shark's intuitive UI. Let me run it again and get back to you. > > Or I could just attach some profiles to the bug and everyone can look at whatever interests them. The profiles wouldn't actually help answer my query, as they only show the distribution of time spent in functions, not the total time spent running some specific benchmark. See the PerformanceTest/ directory for examples of benchmarks. We have many others which are not checked in due to licensing restrictions (ask jamesr or dglaskov for how to run the PLT for example)

Comment on attachment 84983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84983&action=review Some of my comments below, are just that: comments. And not necessarily requiring action, but I thought it useful to write down my thoughts about the patch as I had them. In general, this is great! I'm ready to r+ this as soon as you have PLT numbers or some other performance test which shows that the total time for this is either unchanged, or better. Unfortunately the samples you posted (although interesting!) do not prove that the overall execution time is less with this patch applied (which is the important part). You should also be sure to document the rational and performance improvement in the ChangeLog. (Basically your ChangeLog is very sparse. It's the first thing historians look at when trying to understand what you did, so It's nice to explain yourself. I look at them all the time when looing back through annotate logs.) > Source/WebCore/css/CSSStyleApplyProperty.cpp:64 > + virtual void inherit(CSSStyleSelector* instance) const > + { > + (instance->style()->*m_setter)((instance->parentStyle()->*m_getter)()); > + } I worry a little about this double-indirection with the function pointer *and* the virtual function. Wouldn't it be faster to skip the virtual function and always just use function pointers? Then the subclasses would just set the function pointers to the appropriate types, but the "inherit", "intial" and "value" functions themselves would just always be like this "default" one and not be virtual? Maybe I'm missing something the virtual step is adding. I'm not sure what I mention above is better... but I'd be interested to hear your thoughts on the matter. > Source/WebCore/css/CSSStyleApplyProperty.cpp:88 > + ApplyPropertyColorBase(const Color& (RenderStyle::*getter)() const, const Color& (RenderStyle::*backup)() const, void (RenderStyle::*setter)(const Color&)) Oh, I see by "backup" you mean, "in the case the value isn't found?" Seems that is the same as m_initial in the Default class above? Shouldn't this just be called m_initital? Or maybe m_inheritFrom (since it doesn't seem to be used in the initial() case?) > Source/WebCore/css/CSSStyleApplyProperty.cpp:94 > + virtual void inherit(CSSStyleSelector* instance) const Nit: "selector" is probably a more useful arg name than "instance". > Source/WebCore/css/CSSStyleApplyProperty.cpp:104 > + Color col; "color" here. > Source/WebCore/css/CSSStyleApplyProperty.cpp:109 > + if (value->isPrimitiveValue()) { I think we generally prefer early-return in WebKit. I can't remember if I mentioned this before or not. :) > Source/WebCore/css/CSSStyleApplyProperty.cpp:137 > + CSSPrimitiveValue* p = static_cast<CSSPrimitiveValue*>(value); > + if (p->getIdent() == CSSValueCurrentcolor) Seems we don't need the local for this cast. Again, early return is preferred. Full english names for variables are generally preferred too. These are all nits of course, but add up to general style readability (which for better or worse is very different from Google's). > Source/WebCore/css/CSSStyleApplyProperty.cpp:169 > + setPropertyValue(CSSPropertyBorderBottomColor, new ApplyPropertyColorBase(&RenderStyle::borderBottomColor, &RenderStyle::color, &RenderStyle::setBorderBottomColor)); > + setPropertyValue(CSSPropertyBorderLeftColor, new ApplyPropertyColorBase(&RenderStyle::borderLeftColor, &RenderStyle::color, &RenderStyle::setBorderLeftColor)); > + setPropertyValue(CSSPropertyBorderRightColor, new ApplyPropertyColorBase(&RenderStyle::borderRightColor, &RenderStyle::color, &RenderStyle::setBorderRightColor)); > + setPropertyValue(CSSPropertyBorderTopColor, new ApplyPropertyColorBase(&RenderStyle::borderTopColor, &RenderStyle::color, &RenderStyle::setBorderTopColor)); > + setPropertyValue(CSSPropertyOutlineColor, new ApplyPropertyColorBase(&RenderStyle::outlineColor, &RenderStyle::color, &RenderStyle::setOutlineColor)); > + setPropertyValue(CSSPropertyWebkitColumnRuleColor, new ApplyPropertyColorBase(&RenderStyle::columnRuleColor, &RenderStyle::color, &RenderStyle::setColumnRuleColor)); > + setPropertyValue(CSSPropertyWebkitTextEmphasisColor, new ApplyPropertyColorBase(&RenderStyle::textEmphasisColor, &RenderStyle::color, &RenderStyle::setTextEmphasisColor)); > + setPropertyValue(CSSPropertyWebkitTextFillColor, new ApplyPropertyColorBase(&RenderStyle::textFillColor, &RenderStyle::color, &RenderStyle::setTextFillColor)); > + setPropertyValue(CSSPropertyWebkitTextStrokeColor, new ApplyPropertyColorBase(&RenderStyle::textStrokeColor, &RenderStyle::color, &RenderStyle::setTextStrokeColor)); It seems these still could be made cleaner with a helper. Given how many things have to be pulled from RenderStyle it almost feels like these should be created by some method on RenderStyle. The Base cases certainly could be some helper function: colorPropertyWithBackup(CSSPropertyWebkitTextStrokeColor, textStrokeColor, setTextStrokeColor) but of course that could only be that short if it was actually on RenderStyle. I'm not sure how that would look. This is much nicer than before though, thank you.

IMO the most important thing for this patch are the perf numbers. We can always iterate from here. Anything is better than the current switch of doom. :)

Comment on attachment 84983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84983&action=review > Source/WebCore/css/CSSStyleApplyProperty.cpp:159 > + setPropertyValue(CSSPropertyColor, new ApplyPropertyColor(&RenderStyle::color, &RenderStyle::setColor, RenderStyle::initialColor)); I wonder if these even need to/want to be heap allocated. I'm not sure it really matters, since we allocate them once, up-front. But it seems this is just a jump table you're making, and you could use either struct-allocation, or just copy construct stack objects. I guess that wouldn't work with the current design of using virtual functions however. Again, this is all stuff we can iterate on. I stongly support moving to a more object oriented approach which this patch moves us towards.

Thanks for the review Eric, I'm in meetings today but will try to address them fully tomorrow. In the mean time I just wanted to add some corrected profiling results, since I already have them. Unmodified applyProperty: self: 483 total: 753 self: 404 total: 671 self: 406 total: 665 self: 433 total: 706 self: 430 total: 687 avg total: 696.4 Modified applyProperty: self: 296 total: 513 self: 345 total: 637 self: 393 total: 660 self: 372 total: 687 self: 389 total: 694 avg total: 638.2 I'll look into running those benchmarks tomorrow.

Looks like a good progression. What kind of test case are you using? Assuming 1ms sampling rate, 0.7s of CPU time under a single function is a lot.

(In reply to comment #40) > Looks like a good progression. What kind of test case are you using? Assuming 1ms sampling rate, 0.7s of CPU time under a single function is a lot. See comment #28 for a description of the setup. It was using a 20uS sample rate as 1ms didn't yield a significant number of samples.

Created attachment 85506 [details] Patch

Not much to review w/o performance numbers. (Or responses to previous review comments)

Hi Eric, I think this last patch addresses all the code comments so far. As for PLT, jamesr tells me these are the apple equivalent of the chromium page cycler tests. If you look at https://bugs.webkit.org/show_bug.cgi?id=46592#c45 you'll see the results for those - I didn't find any performance change, and that is why we were looking further at the profiler data.

Comment on attachment 85506 [details] Patch If this doesn't make things slower, it certainly doesn't make the code uglier, so I'm fine with it.

Comment on attachment 85506 [details] Patch Rejecting attachment 85506 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2 Last 500 characters of output: tching file Source/WebCore/css/CSSStyleApplyProperty.cpp patching file Source/WebCore/css/CSSStyleApplyProperty.h patching file Source/WebCore/css/CSSStyleSelector.cpp Hunk #3 succeeded at 3632 (offset 18 lines). Hunk #4 succeeded at 4052 (offset 18 lines). patching file Source/WebCore/css/CSSStyleSelector.h patching file Source/WebCore/rendering/style/RenderStyle.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/8148363

I'm sure the failure is due to the line-endings + svn-apply + vcproj bug :(

Comment on attachment 85506 [details] Patch Rejecting attachment 85506 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2 Last 500 characters of output: tching file Source/WebCore/css/CSSStyleApplyProperty.cpp patching file Source/WebCore/css/CSSStyleApplyProperty.h patching file Source/WebCore/css/CSSStyleSelector.cpp Hunk #3 succeeded at 3632 (offset 18 lines). Hunk #4 succeeded at 4052 (offset 18 lines). patching file Source/WebCore/css/CSSStyleSelector.h patching file Source/WebCore/rendering/style/RenderStyle.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/8141445

(In reply to comment #48) > Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1 As Eric remarked, I suspect this failure is related to bug #53625. I'll take a look later today at latest by the end of this weekend. I was able to use svn-apply to apply this patch successfully in my own SVN working copy. For now, this patch should be landed by hand.

Comment on attachment 85506 [details] Patch Clearing flags on attachment: 85506 Committed r80893: <http://trac.webkit.org/changeset/80893>

All reviewed patches have been landed. Closing bug.

Comment on attachment 85506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85506&action=review As the idea is to make the code more readable, I have a few suggestions: > Source/WebCore/css/CSSStyleApplyProperty.cpp:48 > + ApplyPropertyDefault(T (RenderStyle::*getter)() const, void (RenderStyle::*setter)(T), T (*initial)()) Use typedefs to avoid repeating the function pointers. typedef T (RenderStyle::*GetterMethod)() const; typedef T void (RenderStyle::*SetterMethod)(T); typedef T (*initialMethod)(); .. protected: GetterMethod m_getter; .. > Source/WebCore/css/CSSStyleApplyProperty.cpp:57 > + { > + (selector->style()->*m_setter)((selector->parentStyle()->*m_getter)()); While you're at it: I don't think ASSERTIONS() will hurt here. > Source/WebCore/css/CSSStyleApplyProperty.cpp:65 > + virtual void value(CSSStyleSelector* selector, CSSValue* value) const Why is this named, value, instead of setValue? Or even better, I think, you should have used: applyInheritValue() (maybe s/Value//) applyInitialValue() (ditto) applyValue() > Source/WebCore/css/CSSStyleApplyProperty.cpp:67 > + if (value->isPrimitiveValue()) Early return style is prefereed: if (!value->isPrimitiveValue()) return; > Source/WebCore/css/CSSStyleApplyProperty.cpp:78 > +// CSSPropertyColor > +class ApplyPropertyColorBase : public ApplyPropertyBase { I don't think this is a good idea at all. It should go into a new file. Otherwhise your CSSStyleApplyProperty file will look just as cluttered as CSSStyleSelector is now. > Source/WebCore/css/CSSStyleApplyProperty.cpp:80 > + ApplyPropertyColorBase(const Color& (RenderStyle::*getter)() const, const Color& (RenderStyle::*defaultValue)() const, void (RenderStyle::*setter)(const Color&)) Use typedefs for the function pointers. > Source/WebCore/css/CSSStyleApplyProperty.cpp:88 > + const Color& color = (selector->parentStyle()->*m_getter)(); I'd think ASSERTIONS would be helpful as well here. > Source/WebCore/css/CSSStyleApplyProperty.cpp:97 > + (selector->style()->*m_setter)(color); Ditto. > Source/WebCore/css/CSSStyleApplyProperty.cpp:102 > + if (value->isPrimitiveValue()) > + (selector->style()->*m_setter)(selector->getColorFromPrimitiveValue(static_cast<CSSPrimitiveValue*>(value))); Ditto + Early return. > Source/WebCore/css/CSSStyleApplyProperty.cpp:110 > +class ApplyPropertyColor : public ApplyPropertyColorBase { Why is this seperation needed ApplyPropertyColor and base? It's highly confusing IMHO. Imagine how many classes we'd end up creating, when converting everything into your new approach. We should rather minimize the need of new classes. > Source/WebCore/css/CSSStyleApplyProperty.cpp:149 > + setPropertyValue(CSSPropertyColor, new ApplyPropertyColor(&RenderStyle::color, &RenderStyle::setColor, RenderStyle::initialColor)); I'd rather prefer to move the setPropertyValue() calls in a new CSSStyleApplyPropertyColor.cpp file, in a static function. So that CSSStyleApplyProperty only ever has to look like this: CSSStyleApplyPropertyColor::initialize(this); CSSStyleApplyPropertyXXX::initialize(this); ... For each of the groups of properties. > Source/WebCore/css/CSSStyleApplyProperty.h:47 > +class CSSStyleApplyProperty : public RefCounted<CSSStyleApplyProperty> { Why is this refcounted if you plan to share a static single instance?? > Source/WebCore/css/CSSStyleApplyProperty.h:51 > + void inherit(CSSPropertyID property, CSSStyleSelector* selector) const Same renamings would be nice. Sorry, I had no chance to post earlier, I'm kind of ill.

Luke, Eric, any comments?

(In reply to comment #53) > Luke, Eric, any comments? Just got back from leave today - will go through it shortly.

Comment on attachment 85506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85506&action=review >> Source/WebCore/css/CSSStyleApplyProperty.cpp:48 >> + ApplyPropertyDefault(T (RenderStyle::*getter)() const, void (RenderStyle::*setter)(T), T (*initial)()) > > Use typedefs to avoid repeating the function pointers. > > typedef T (RenderStyle::*GetterMethod)() const; > typedef T void (RenderStyle::*SetterMethod)(T); > typedef T (*initialMethod)(); > > .. > protected: > GetterMethod m_getter; > .. Yeah, that seems significantly easier to handle. >> Source/WebCore/css/CSSStyleApplyProperty.cpp:57 >> + (selector->style()->*m_setter)((selector->parentStyle()->*m_getter)()); > > While you're at it: I don't think ASSERTIONS() will hurt here. What would you ASSERT() here? >> Source/WebCore/css/CSSStyleApplyProperty.cpp:67 >> + if (value->isPrimitiveValue()) > > Early return style is prefereed: > if (!value->isPrimitiveValue()) > return; Early return for a 2-line function is not beneficial. It turns a 2-line function into a 3-line function that is no easier to understand.

Comment on attachment 85506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85506&action=review > Source/WebCore/css/CSSStyleSelector.cpp:3627 > + if (m_applyProperty.implements(property)) { > + if (isInherit) > + m_applyProperty.inherit(property, this); > + else if (isInitial) > + m_applyProperty.initial(property, this); > + else > + m_applyProperty.value(property, this, value); > + return; WebKit uses 4-space indentation only.

I'm reopening the bug. Most of the issues I commented when reviewing, have not been fixed. New code is being added to CSSStyleProperty now, and I still dislike the structure. Could you fix the existing issues first, before moving new props to the new concept? Especially the CSSStyleApplyProperty.cpp file that now contains several template classes is confusing. What I mean here is: we had one big file with lots of switch/case statements, now we have a new file, that will contain a template class for each property - that's not a benefit in any way. We want to have seperated files for each property, even better in a new subdirectory in css, making it more maintainable. Please lets not degenerate CSSStyleApplyProperty to a does-it-all-class. I still want to see the typedefs for the function pointers, renames for the inherit/initial functions etc... :(

(In reply to comment #57) > We want to have seperated files for each property, even better in a new subdirectory in css, making it more maintainable. I certainly don't want a new file for every property. That's insane!

(In reply to comment #58) > (In reply to comment #57) > > We want to have seperated files for each property, even better in a new subdirectory in css, making it more maintainable. > > I certainly don't want a new file for every property. That's insane! I should have said for each logical group of properties. What's really insane is an all-in-one-file handling _every_ property. Think about what happens if we move all of the current CSS properties to the new concept. The CSSStyleApplyProperty file would grow sth. like 10 times compared to the current CSSStyleSelector - a new class for each property.... that does NOT scale well at all. I still like to hear a resolution if my comments are being adressed, finally? I commented almost a month ago.

I didn't want to reply earlier because I didn't really know what to say - I'm pretty sure it will come off as excuses to avoid work, but let me assure you that isn't the case, and humbly offer the reasons why I didn't follow all of these suggestions. (In reply to comment #52) > (From update of attachment 85506 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=85506&action=review > > As the idea is to make the code more readable, I have a few suggestions: > > > Source/WebCore/css/CSSStyleApplyProperty.cpp:48 > > + ApplyPropertyDefault(T (RenderStyle::*getter)() const, void (RenderStyle::*setter)(T), T (*initial)()) > > Use typedefs to avoid repeating the function pointers. > > typedef T (RenderStyle::*GetterMethod)() const; > typedef T void (RenderStyle::*SetterMethod)(T); > typedef T (*initialMethod)(); > > .. > protected: > GetterMethod m_getter; > .. I don't think this buys much other than obfuscating the types, since these typedefs would only be used approximately twice, and need to be defined in every class. I acknowledge this as an aesthetic preference, however my previous experience has been that proliferation of typedefs was a net negative in code readability. > > Source/WebCore/css/CSSStyleApplyProperty.cpp:57 > > + { > > + (selector->style()->*m_setter)((selector->parentStyle()->*m_getter)()); > > While you're at it: I don't think ASSERTIONS() will hurt here. It is not clear what exactly you would want to assert here. Assertions are useful to early-out if an unexpected condition occurs, however in this case the only unexpected condition will immediately result in a null pointer dereference, so no additional assertion is necessary. > > Source/WebCore/css/CSSStyleApplyProperty.cpp:65 > > + virtual void value(CSSStyleSelector* selector, CSSValue* value) const > > Why is this named, value, instead of setValue? Or even better, I think, you should have used: > applyInheritValue() (maybe s/Value//) > applyInitialValue() (ditto) > applyValue() Ok, expect a patch for this shortly. > > Source/WebCore/css/CSSStyleApplyProperty.cpp:67 > > + if (value->isPrimitiveValue()) > > Early return style is prefereed: > if (!value->isPrimitiveValue()) > return; I had discussed this with two other reviewers who both agreed that early return was not preferred for functions that contain a single short if statement. > > Source/WebCore/css/CSSStyleApplyProperty.cpp:78 > > +// CSSPropertyColor > > +class ApplyPropertyColorBase : public ApplyPropertyBase { > > I don't think this is a good idea at all. It should go into a new file. Otherwhise your CSSStyleApplyProperty file will look just as cluttered as CSSStyleSelector is now. My thoughts were along the lines of smfr's comment - Having this logic spread over a large number of files may well make it less readable, especially since this classes are effectively private to CSSStyleApplyProperty. There may not be one class per property, but there will be quite a few when this refactoring is complete. I would be in favor of doing the refactoring with all the private command pattern classes in a single file, at which point we will know exactly how many there are, and how interrelated they are, and will be in a better position to decide on whether they should be split into separate files or not. > > Source/WebCore/css/CSSStyleApplyProperty.cpp:80 > > + ApplyPropertyColorBase(const Color& (RenderStyle::*getter)() const, const Color& (RenderStyle::*defaultValue)() const, void (RenderStyle::*setter)(const Color&)) > > Use typedefs for the function pointers. See comment above. > > Source/WebCore/css/CSSStyleApplyProperty.cpp:88 > > + const Color& color = (selector->parentStyle()->*m_getter)(); > > I'd think ASSERTIONS would be helpful as well here. See comment above. > > Source/WebCore/css/CSSStyleApplyProperty.cpp:97 > > + (selector->style()->*m_setter)(color); > > Ditto. > > > Source/WebCore/css/CSSStyleApplyProperty.cpp:102 > > + if (value->isPrimitiveValue()) > > + (selector->style()->*m_setter)(selector->getColorFromPrimitiveValue(static_cast<CSSPrimitiveValue*>(value))); > > Ditto + Early return. > > > Source/WebCore/css/CSSStyleApplyProperty.cpp:110 > > +class ApplyPropertyColor : public ApplyPropertyColorBase { > > Why is this seperation needed ApplyPropertyColor and base? It's highly confusing IMHO. > > Imagine how many classes we'd end up creating, when converting everything into your new approach. > We should rather minimize the need of new classes. All I can say is that this needs to be evaluated on a case-by-case basis. ApplyPropertyColor and ApplyPropertyColorBase only share applyInheritValue, the logic of this isn't something I came up with, it's just that the color css property has different behavior to other properties that take color values. > > Source/WebCore/css/CSSStyleApplyProperty.cpp:149 > > + setPropertyValue(CSSPropertyColor, new ApplyPropertyColor(&RenderStyle::color, &RenderStyle::setColor, RenderStyle::initialColor)); > > I'd rather prefer to move the setPropertyValue() calls in a new CSSStyleApplyPropertyColor.cpp file, in a static function. > So that CSSStyleApplyProperty only ever has to look like this: > CSSStyleApplyPropertyColor::initialize(this); > CSSStyleApplyPropertyXXX::initialize(this); > ... > > For each of the groups of properties. The reason not to do this is that CSSStyleSelector and RenderStyle, and possibly others would need to friend a ton of classes. By passing parameters from CSSStyleApplyProperty you keep all the friend nastiness confined to a single place. > > Source/WebCore/css/CSSStyleApplyProperty.h:47 > > +class CSSStyleApplyProperty : public RefCounted<CSSStyleApplyProperty> { > > Why is this refcounted if you plan to share a static single instance?? Fixed. > > Source/WebCore/css/CSSStyleApplyProperty.h:51 > > + void inherit(CSSPropertyID property, CSSStyleSelector* selector) const > > Same renamings would be nice. Will do, expect a CL soon.

Sampling show that CSSStyleApplyProperty has caused a massive regression in applyProperty, making it at least two times slower. In practical terms this slows down page load of http://www.whatwg.org/specs/web-apps/current-work/ by more than a second on i7. The regression is largely caused by the introduction of virtual calls to a critical code path. The original patch probably didn't show regression due to handling very few properties (performance was discussed in bug 46592). Stylistically CSSStyleApplyProperty is pretty appalling jumble of jumps tables, virtual calls, function pointers and templates, a clear regression from the clarity of the original switch statement. The switch statement still exists in any case as this work was left in half-done state. This patch and the subsequent moving of properties needs to be reverted.

(In reply to comment #61) > Sampling show that CSSStyleApplyProperty has caused a massive regression in applyProperty, making it at least two times slower. In practical terms this slows down page load of http://www.whatwg.org/specs/web-apps/current-work/ by more than a second on i7. Ouch. > The regression is largely caused by the introduction of virtual calls to a critical code path. I very much believe you. This risk was discussed in this and other bugs. Do you have a particular favorite tool which shows you this result, or is this derived from other data and your profiling experience? If so, could you list the tools you used so that others (most importantly Luke) could also reproduce your results? > The original patch probably didn't show regression due to handling very few properties (performance was discussed in bug 46592). > > Stylistically CSSStyleApplyProperty is pretty appalling jumble of jumps tables, virtual calls, function pointers and templates, a clear regression from the clarity of the original switch statement. The switch statement still exists in any case as this work was left in half-done state. I'm not sure this stone is worth throwing. > This patch and the subsequent moving of properties needs to be reverted. What I hear you saying is that this performance regression needs to be adressed. Revert is obviously one course of action, possibly the best one. It seems a slightly premature conclusion unless you've spoken with the patch author? (I have not, I only know him through IRC.) Thank you for providing a concrete test case. On possible addressing of said regression would be to revert all this work of course. I do not have enough data (besides of course your trusted -- but possibly hasty? -- recommendation) to know if that is the best course of action. Luke: I would recommend that you reproduce the regression locally under a sampler and update the bug on how you plan to proceed. We obviously can't leave such a huge regression in the tree for any length of time. Antti: Have you seen any slowdown in other benchmarks which your team may run more regularly, like the PLT?

FWIW, at this point reverting won't work because new CSS properties have been added that are only in the lookup table. If we go back to the switch statement, we'll have to port this code.

(In reply to comment #63) > FWIW, at this point reverting won't work because new CSS properties have been added that are only in the lookup table. If we go back to the switch statement, we'll have to port this code. Why can't the new entries just be added to the old switch?

(In reply to comment #64) > (In reply to comment #63) > > FWIW, at this point reverting won't work because new CSS properties have been added that are only in the lookup table. If we go back to the switch statement, we'll have to port this code. > > Why can't the new entries just be added to the old switch? They can, I'm just saying it's not as simple as running webkit-patch rollout or git revert.

There is a lot to respond to here, so apologies if I miss anything. This isn't work left half done, it's just work in progress. I'm still chipping away at it and it is a big task. Stylistically there are pros and cons each way, the switch statement and extensive use of macros is not great either. I've certainly found a lot of bugs and duplicated code, and I would say that we have gained a lot from improved type safety and reduced code duplication. It does need cleaning up and splitting out into multiple files though. I think Eric has made some good points, if you could explain how you determined that there was a performance regression here in a way that I can replicate, I would be happy to take a look at it. Also, if there are other other options in the design space that should be considered, then perhaps this would be a good opportunity to do that.

(In reply to comment #62) > I very much believe you. This risk was discussed in this and other bugs. Do you have a particular favorite tool which shows you this result, or is this derived from other data and your profiling experience? If so, could you list the tools you used so that others (most importantly Luke) could also reproduce your results? I was using Instruments to look the profile of page load of http://www.whatwg.org/specs/web-apps/current-work/. Very large sample counts show up in the virtual function call entry points. Not that that page does nothing special in terms styling, it is just big and complex enough to make style resolve performance easily observable on desktop computers. On ARM pretty much any complex page displays similar characteristics, observable by sampling. > > Stylistically CSSStyleApplyProperty is pretty appalling jumble of jumps tables, virtual calls, function pointers and templates, a clear regression from the clarity of the original switch statement. The switch statement still exists in any case as this work was left in half-done state. > > I'm not sure this stone is worth throwing. Let me put it this way. Style applying is analogous to HTML tree building. We have a stream of property tokens as input and we parse them to build up a RenderStyle object as output. You worked on HTML5 parser. The tree builder is implemented as large switch statements. Would you refactor it to use a lookup table (one slot per token) of instances of virtual classes? Probably not. Yet the code here is often more important for performance than the HTML parser (> 2s vs < 0.5s over the spec load). I think architecturally this work has moved us to wrong direction and this is an important aspect of the discussion. Refactoring is costly (including the cost of discussion we are now having) and should not be attempted unless benefits are clear. > What I hear you saying is that this performance regression needs to be adressed. Revert is obviously one course of action, possibly the best one. It seems a slightly premature conclusion unless you've spoken with the patch author? (I have not, I only know him through IRC.) > > Thank you for providing a concrete test case. On possible addressing of said regression would be to revert all this work of course. I do not have enough data (besides of course your trusted -- but possibly hasty? -- recommendation) to know if that is the best course of action. This is currently blocking us from doing the sensible refactoring (moving the style applying to a separate class and file as was done for SelectorChecker) as that would make any future reverting more complicated. Going back to switch looks like one of the easiest to achieve major performance progressions we have in the engine right now (purely mechanical work essentially) and would be worth doing on those ground alone. > Antti: Have you seen any slowdown in other benchmarks which your team may run more regularly, like the PLT? Not that I know of. There have been progressions in selector matching performance big enough to easily mask out any regressions here.

To be clear: This regression is important to myself (and others at my current place of work). I will look at reproducing Antti's results this afternoon and creating a CSS micro-benchmark from this page, akin to the ones we have for the HTML5 parser: http://trac.webkit.org/browser/trunk/PerformanceTests Last I spoke with Luke he was out of the (Sydney) office with the flu. I expect we'll hear more from him when he returns. In any case, I will look into resolving this regression. I have perhaps not payed as much attention to this work as I should have. You can expect regular updates to this bug on weekdays (Monday-Friday) until this regression is resolved.

No updates. I will not have updates tomorrow, but you can expect to hear from me Monday.

Spent some time working on a benchmark today. Hope to post one tomorrow.

(In reply to comment #70) > Spent some time working on a benchmark today. Hope to post one tomorrow. @Antti: I haven't been able to get meaningful profiling out of shark due to missing symbols, and others have been having the same problem. What profiling setup are you using to measure this?

Are you profiling Chrome or Safari? build-webkit --release && run-safari --release should give you a menaingful stack. At least it used to.

(In reply to comment #72) > Are you profiling Chrome or Safari? build-webkit --release && run-safari --release should give you a menaingful stack. At least it used to. It used to work for me, but apparently stopped working at some point. Ojan and Jamesr both confirmed they haven't been able to get it to work recently either, so it's not just me.

I have better luck with Instruments than Shark these days.

Thanks for the tip smfr. Running Instruments against WebProcess is working well for me now. It's quite a nice tool too :)

Using a Time Profile in Instruments against WebProcess, using a Release version of r96142, I too am seeing CSSStyleSelector::applyProperty() as the hottest symbol. :( Attaching the files I'm working from now.

I can't actually test on my work machine because of radar 9077267/8596604 (instruments panicks). I'll upgrade to Lion and pray.

Created attachment 108928 [details] Shark trace of loading the HTML specification. I've added a shark trace of the page load that Antti is talking about (http://www.whatwg.org/specs/web-apps/current-work/). The sum of self samples in ApplyProperty*::apply* functions is about 2300. The number of self samples in CSSStyleSelector::applyProperty is about 9100, 1200 of which are looking up and calling the apply property functions. So CSSStyleSelector::applyProperty accounts for 7900 samples if the calls to CSSStyleApplyProperty are removed, and CSSStyleApplyProperty and call sites account for 3500 samples in total. Note: ~115 CSS properties are implemented in CSSStyleSelector::applyProperty ~145 CSS properties are implemented in CSSStyleApplyProperty (disclaimer: I counted by hand and discarded things that were #ifdef-ed out etc). I don't know the exact distribution of css properties used by this page, but it is reasonable to assume that CSSStyleApplyProperty is contributing a significant amount of the actual work done. The data I'm seeing has not yet convinced me that this is a "massive regression in applyProperty, making it at least two times slower", but I do agree that we could optimise this code further by refactoring to eliminate or reduce the use virtual functions.

I think the mistake (and this is on me, since I was consulted before this work began) here is that this work probably should have been prototyped first and performance/memory tested after all properties were converted. Changing this incrementally has been taking far too long, and the result is the tree has been left in a half-converted state for a really long time. I also am of the opinion that the new code is adding a lot of complexity and is pretty hard to follow. I'll confess that the many new CSS properties that I've added have all been to the switch statement, because the other code is more complicated. Let's face it. Advanced C++ can lead to some really inscrutable code, and I'm not really a fan of how this has all panned out. My vote would be that we revert to the original approach of a giant switch statement and this work could perhaps continue on a branch or in a prototype and have to prove itself performance and footprint-wise before becoming the approach adopted by all properties.

My apologies for failing to note: I added a benchmark to model HTML5 spec load times in https://bugs.webkit.org/show_bug.cgi?id=69374. If that benchmark can be trusted (which I believe it can), the sketched-out revert patch from Antti was actually slower than current tip of tree: https://bugs.webkit.org/show_bug.cgi?id=68944#c13. Luke is continuing an investigation into improving performance of HTML5 spec load times (and that benchmark), and the work can be tracked in https://bugs.webkit.org/show_bug.cgi?id=68944. I suspect we can close this bug now, and track any further work in bug 68944. Of course, please re-open if you feel differently.