Fix MQEvaluator::mediaType() related perf regressions.

Fix MQEvaluator::mediaType() related perf regressions.

When we stopped storing the mediaType as a string on m_medium on StyleResolver, 
we started calling FrameView::mediaType() every time that the mediaType as required.
In the related bug's test case, mediaType() was being called a lot, 
in order to compare it to "print" to see which default style sheet should be used.
Since mediaType() calls also call InspectorInstrumentation::applyEmulatedMedia(), 
they have a certain overhead, that impacted the benchmark's results.

I've changed it so that the mediaType() call and the comparison to "print"
would be done once, and the result cached.

BUG=394275
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179991

[esprehn, 2014-07-21 15:22:32]

This makes us depend on RVO, and all you're saving is some ref counting, I doubt
that's the reason for a 10% perf hit. I don't think we should land this.

[Yoav Weiss, 2014-07-21 15:32:37]

On 2014/07/21 15:22:32, esprehn wrote:
> This makes us depend on RVO, and all you're saving is some ref counting, I
doubt
> that's the reason for a 10% perf hit. I don't think we should land this.

Yeah, I also reached the conclusion that this patch is not good, since
FrameView::mediaType() is not returning a reference, and referencing that is
causing all kinds of trouble. (specifically unit test crashes on linux debug).

I'll modify the patch so that the test in StyleResolver to see if the current
mediaType is "print" (which AFAICT is the only call this test makes to the MQ
eval code that I've touched in the perf regressing CL), will go directly to
FrameView::mediaType()

[esprehn, 2014-07-25 23:42:04]

This still doesn't make sense.

https://codereview.chromium.org/405953002/diff/40001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/405953002/diff/40001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:483: RuleSet* userAgentStyleSheet =
printMediaType(document().view())
Why would this be slow? mediaTypeMatchSpecific just compares the string right?

[Yoav Weiss, 2014-07-26 04:56:26]

On 2014/07/25 23:42:04, esprehn wrote:
> This still doesn't make sense.
> 
>
https://codereview.chromium.org/405953002/diff/40001/Source/core/css/resolver...
> File Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/405953002/diff/40001/Source/core/css/resolver...
> Source/core/css/resolver/StyleResolver.cpp:483: RuleSet* userAgentStyleSheet =
> printMediaType(document().view())
> Why would this be slow? mediaTypeMatchSpecific just compares the string right?

mediaTypeMatchSpecific just compares the string, but it gets it from
MediaValuesDynamic, which calls MediaValues::calculateMediaType, which calls
frame()->view()->mediaType().
This print comparison seems to be called a lot for the test in question, so I
tried to take a shortcut here by calling a single static function instead.

The only reason I think it may help is this is the only function that I found
that is influenced by the mediaType changes patch and is called by the regressed
test.

[esprehn, 2014-07-30 05:05:54]

On 2014/07/26 at 04:56:26, yoav wrote:
> On 2014/07/25 23:42:04, esprehn wrote:
> ...
> 
> mediaTypeMatchSpecific just compares the string, but it gets it from
MediaValuesDynamic, which calls MediaValues::calculateMediaType, which calls
frame()->view()->mediaType().
> This print comparison seems to be called a lot for the test in question, so I
tried to take a shortcut here by calling a single static function instead.
> 
> The only reason I think it may help is this is the only function that I found
that is influenced by the mediaType changes patch and is called by the regressed
test.

There's no reason to think that would be slow either. I really think you need to
profile this or we should consider rolling that patch out. Comparing and
returning AtomicStrings shouldn't be making it 10% slower.

[Yoav Weiss, 2014-08-11 20:09:35]

On 2014/07/30 05:05:54, esprehn wrote:
> On 2014/07/26 at 04:56:26, yoav wrote:
> > On 2014/07/25 23:42:04, esprehn wrote:
> > ...
> > 
> > mediaTypeMatchSpecific just compares the string, but it gets it from
> MediaValuesDynamic, which calls MediaValues::calculateMediaType, which calls
> frame()->view()->mediaType().
> > This print comparison seems to be called a lot for the test in question, so
I
> tried to take a shortcut here by calling a single static function instead.
> > 
> > The only reason I think it may help is this is the only function that I
found
> that is influenced by the mediaType changes patch and is called by the
regressed
> test.
> 
> There's no reason to think that would be slow either. I really think you need
to
> profile this or we should consider rolling that patch out. Comparing and
> returning AtomicStrings shouldn't be making it 10% slower.

I've found the real cause of this regression. It was the fact that
FrameView::mediaType() was called on each comparison to "print", rather than
cached as a string.
I've updated the CL details accordingly.

[esprehn, 2014-08-11 20:33:21]

This seems okay, but what updates that bit when you change from non-print ->
print? Do we always throw away the style resolver?

https://codereview.chromium.org/405953002/diff/80001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/405953002/diff/80001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:145: m_printMediaType =
equalIgnoringCase(view->mediaType(), MediaTypeNames::print);
Do we destroy the StyleResolver when the media type changes from non-print to
print and then back?

[Yoav Weiss, 2014-08-11 20:38:12]

On 2014/08/11 20:33:21, esprehn wrote:
> This seems okay, but what updates that bit when you change from non-print ->
> print? Do we always throw away the style resolver?
> 
>
https://codereview.chromium.org/405953002/diff/80001/Source/core/css/resolver...
> File Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/405953002/diff/80001/Source/core/css/resolver...
> Source/core/css/resolver/StyleResolver.cpp:145: m_printMediaType =
> equalIgnoringCase(view->mediaType(), MediaTypeNames::print);
> Do we destroy the StyleResolver when the media type changes from non-print to
> print and then back?

I'm not 100% sure, but I believe so, since before
https://codereview.chromium.org/335313003 landed, the mediaType was already
cached as a string inside m_medium, which is created at the StyleResolver's
constructor.

[esprehn, 2014-08-11 20:40:39]

lgtm, okay

[Yoav Weiss, 2014-08-11 20:52:16]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-08-11 20:52:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoav@yoav.ws/405953002/80001

[Yoav Weiss, 2014-08-11 21:04:46]

On 2014/08/11 20:38:12, Yoav Weiss wrote:
> On 2014/08/11 20:33:21, esprehn wrote:
> > This seems okay, but what updates that bit when you change from non-print ->
> > print? Do we always throw away the style resolver?
> > 
> >
>
https://codereview.chromium.org/405953002/diff/80001/Source/core/css/resolver...
> > File Source/core/css/resolver/StyleResolver.cpp (right):
> > 
> >
>
https://codereview.chromium.org/405953002/diff/80001/Source/core/css/resolver...
> > Source/core/css/resolver/StyleResolver.cpp:145: m_printMediaType =
> > equalIgnoringCase(view->mediaType(), MediaTypeNames::print);
> > Do we destroy the StyleResolver when the media type changes from non-print
to
> > print and then back?
> 
> I'm not 100% sure, but I believe so, since before
> https://codereview.chromium.org/335313003 landed, the mediaType was already
> cached as a string inside m_medium, which is created at the StyleResolver's
> constructor.

Yeah. Looks like it does.
FrameView:setMediaType() => Document::mediaQueryAffectingValueChanged() =>
StyleEngine::clearMediaQueryRuleSetStyleSheets() =>
TreeScopeStyleSheetCollection::clearMediaQueryRuleSetStyleSheets() =>
StyleSheetContent::clearRuleSet()=> StyleSheetContent::clearResolvers()

[commit-bot: I haz the power, 2014-08-11 21:10:50]

Change committed as 179991