Optimize StyleSheetContents::checkLoaded by keeping track of completed

Optimize StyleSheetContents::checkLoaded by keeping track of completed
and loading clients.

For the input-wstyle.html benchmark from the bug report this brings
the difference between the x-input and the x-input-style down to
a factor of two and the time scales linearly with the number of
elements added in the benchmark.

x-input: 52.1520ms
x-input-style: 105.3900ms
input: 27.0840ms

R=adamk@chromium.org, esprehn@chromium.org, haraken@chromium.org, tasak@chromium.org
BUG=350785
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169287

[tasak, 2014-03-13 01:29:31]

I think, this change looks good.

However, I would like to wait for esprehn's comment.

https://codereview.chromium.org/196653006/diff/1/Source/core/css/CSSStyleShee...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/196653006/diff/1/Source/core/css/CSSStyleShee...
Source/core/css/CSSStyleSheet.cpp:419: if (completed != m_loadCompleted) {
Nit: I think, blink loves early return.

[Mads Ager (chromium), 2014-03-13 09:08:22]

https://codereview.chromium.org/196653006/diff/1/Source/core/css/CSSStyleShee...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/196653006/diff/1/Source/core/css/CSSStyleShee...
Source/core/css/CSSStyleSheet.cpp:419: if (completed != m_loadCompleted) {
On 2014/03/13 01:29:31, tasak wrote:
> Nit: I think, blink loves early return.

Done.

[esprehn, 2014-03-13 19:56:27]

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:379: return true;
return rootStyleSheet()->m_loadingClients.isEmpty();

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:413: protectedClients.append(*it);
This should just use copyToVector()

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:457: return root->hasOneClient();
rootStyleSheet()->hasOneClient()

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:464: return root->m_loadingClients.size()
? (*root->m_loadingClients.begin())->ownerNode() :
(*root->m_completedClients.begin())->ownerNode();
Lets break this up since it's easier to read.

if (!root->hasOneClient())
  return 0;
if (root->m_loadingClients.size())
  return ...;
return ...;

you don't need the assert.

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:529: m_loadingClients.add(sheet);
Why is it a HashSet if you're just going to ASSERT that you're not already in
it. Could it be just a Vector?

[Mads Ager (chromium), 2014-03-14 09:30:18]

Thanks for the review! PTAL

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:379: return true;
On 2014/03/13 19:56:28, esprehn wrote:
> return rootStyleSheet()->m_loadingClients.isEmpty();

Heh, yes, thanks!

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:413: protectedClients.append(*it);
On 2014/03/13 19:56:28, esprehn wrote:
> This should just use copyToVector()

Oh, I didn't know about copyToVector. Much better! Done. :)

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:457: return root->hasOneClient();
On 2014/03/13 19:56:28, esprehn wrote:
> rootStyleSheet()->hasOneClient()

Done.

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:464: return root->m_loadingClients.size()
? (*root->m_loadingClients.begin())->ownerNode() :
(*root->m_completedClients.begin())->ownerNode();
On 2014/03/13 19:56:28, esprehn wrote:
> Lets break this up since it's easier to read.
> 
> if (!root->hasOneClient())
>   return 0;
> if (root->m_loadingClients.size())
>   return ...;
> return ...;
> 
> you don't need the assert.

Yes, much better, thanks.

https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:529: m_loadingClients.add(sheet);
On 2014/03/13 19:56:28, esprehn wrote:
> Why is it a HashSet if you're just going to ASSERT that you're not already in
> it. Could it be just a Vector?

The set of clients is a weak set. It does not keep the clients alive and when
they die they remove themselves from the set. So there are random order
removals. For those a vector will perform a lot of copying and a set seems more
reasonable.

Having these be sets makes the move between loading and non-loading that this
change is introducing more efficient as well.

I kept the assertion in order to not losen any requirements that I do not know
about in this code. Also, it seems like a good idea at least to assert that the
client is not going to appear in both the loading and completed set. I could
simplify the assert to just check that the client is not in m_completedClients
when we add it to m_loadingClients.

(There is an oilpan-only reason as well which is that weak processing of vectors
is hard to make sense of and weak processing of HashSets is easy. However, that
is not really relevant to this particular change.)

[esprehn, 2014-03-14 23:18:29]

lgtm

[commit-bot: I haz the power, 2014-03-14 23:18:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/196653006/40001

[commit-bot: I haz the power, 2014-03-14 23:32:41]

Change committed as 169287