Bug 17653 – CSSRuleSet should be moved out of CSSStyleSelector.cpp into its own files

• Component: CSS
• Status: RESOLVED LATER

[Eric Seidel (no email)]

CSSRuleSet should be moved out of CSSStyleSelector.cpp into its own files

I went looking for CSSRuleSet today, and found it wasn't in its own file(s).  It would be nice to see it moved.

[Ryosuke Niwa]

Created attachment 74337 [details]
Extracted CSSRuleSet.h and CSSRuleSet.cpp

[Eric Seidel (no email)]

Do we have a good way to performance test this?

It would be awesome if we could make a CSSStyleSelector micro-benchmark like we have for the html-parser (in WebCore/benchmarks).

It's possible this could cause some of these functions to not inline, which could slow down.  I'm not super concerned, but we just need to make sure we know we're going to catch a problem if this were to regress.

[Eric Seidel (no email)]

Comment on attachment 74337 [details]
Extracted CSSRuleSet.h and CSSRuleSet.cpp

View in context: https://bugs.webkit.org/attachment.cgi?id=74337&action=review

LGTM, assuming you're not changing any logic (just moving) and you have an answer for the perf question.

> WebCore/css/CSSRuleSet.cpp:43
> +: m_ruleCount(0)
> +, m_pageRuleCount(0)

I think we normally indent these.

[Ryosuke Niwa]

(In reply to comment #2)
> Do we have a good way to performance test this?

No.  Maybe we need to make a benchmark.

> It would be awesome if we could make a CSSStyleSelector micro-benchmark like we have for the html-parser (in WebCore/benchmarks).

> It's possible this could cause some of these functions to not inline, which could slow down.  I'm not super concerned, but we just need to make sure we know we're going to catch a problem if this were to regress.

Right.  jamesr pointed that out as well.

(In reply to comment #3)
> LGTM, assuming you're not changing any logic (just moving) and you have an answer for the perf question.

We need to figure out how to perf-test this.

[Eric Seidel (no email)]

It's possible a sufficient answer to the perf question is that the PLT (which I believe chromium runs automatically on some bots) doesn't slow down.  But if you were to come up with a micro benchmark that might be even better. :)

[Ryosuke Niwa]

(In reply to comment #5)
> It's possible a sufficient answer to the perf question is that the PLT (which I believe chromium runs automatically on some bots) doesn't slow down.  But if you were to come up with a micro benchmark that might be even better. :)

Why don't we make the constructor, destructor, and addPageRule inline?  Other functions are too big to be inlined or the cost of not being inline is negligible.  James said inlining might hurt the performance as well but I'm almost certain that the compiler won't keep it inline if that's the case.

[Eric Seidel (no email)]

ping?

[Eric Seidel (no email)]

Looks like this never got landed.

[Ryosuke Niwa]

(In reply to comment #8)
> Looks like this never got landed.

Yeah, I never got around to test the performance :(

[Eric Seidel (no email)]

Don't we have perf try-bots these days?

[Ryosuke Niwa]

This patch is completely out of date at this point.