[webkit-reviews] review denied: [Bug 74863] Web Inspector: Add CSSStyleSelector instrumentation calls towards implementing a CSS selector profiler : [Attachment 119877] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 74863: Web Inspector: Add CSSStyleSelector instrumentation calls towards
implementing a CSS selector profiler
https://bugs.webkit.org/show_bug.cgi?id=74863

Attachment 119877: Patch
https://bugs.webkit.org/attachment.cgi?id=119877&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=119877&action=review


r- on perf concerns.

> Source/WebCore/css/CSSStyleSelector.cpp:722
> +	   CSSStyleRule* rule = ruleData.rule();
> +	   InspectorInstrumentationCookie cookie =
InspectorInstrumentation::willMatchRule(document(), rule);
> +	   if (canUseFastReject &&
m_checker.fastRejectSelector<RuleData::maximumIdentifierCount>(ruleData.descend
antSelectorIdentifierHashes())) {
> +	       InspectorInstrumentation::didMatchRule(cookie, false);
>	       continue;
> +	   }

As said before, I don't think you should put anything on the fastRejectSelector
path. You are not measuring anything interesting (the selectors rejected by
this loop are not any more relevant than the selectors rejected by earlier hash
lookup optimizations which you don't measure) and very little time is spent
here anyway (bloom filter lookup is very fast).

Extra branches here are very likely to regress performance. If you look at the
assembly, you will see that this code compiles to a very tight loop. You would
need to prove that it doesn't on ARM where this kind of stuff can really
matter.

Other additional checks in this patch are probably fine.