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.
Created attachment 74337 [details] Extracted CSSRuleSet.h and CSSRuleSet.cpp
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.
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.
(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.
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. :)
(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.
ping?
Looks like this never got landed.
(In reply to comment #8) > Looks like this never got landed. Yeah, I never got around to test the performance :(
Don't we have perf try-bots these days?
This patch is completely out of date at this point.