Bug 127039

Summary:

Remove the CSS selector profiler.

Product:

WebKit

Reporter:

Andreas Kling <kling>

Component:

Web Inspector

Assignee:

Andreas Kling <kling>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, esprehn+autocc, glenn, graouts, gyuyoung.kim, joepeck, kling, macpherson, menard, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch with removed files	sam: review+

Description Andreas Kling 2014-01-15 02:45:04 PST

The selector profiler was painting a mostly fictional picture of what
selectors we were spending time on. It never really grokked the fast
path selectors, nor did it understand recent additions like the extra 
cascading pass or the selector JIT.

Somewhat ironically, this may end up making some selectors run faster
since it removes a number of brances in hot code.

FWIW, Blink removed theirs here <https://codereview.chromium.org/21049007>

Comment 1 Radar WebKit Bug Importer 2014-01-15 02:45:26 PST

<rdar://problem/15823608>

Comment 2 Andreas Kling 2014-01-15 02:50:10 PST

Created attachment 221253 [details]
Patch

Comment 3 Andreas Kling 2014-01-15 02:56:54 PST

Created attachment 221255 [details]
Patch with removed files

Comment 4 Antti Koivisto 2014-01-15 03:06:58 PST

Comment on attachment 221255 [details]
Patch with removed files

Selector profiles are somewhere between useless and actively harmful unless you know a ton about engine internals. Removal is a good idea.

Comment 5 Timothy Hatcher 2014-01-15 11:26:36 PST

Sounds good. I never liked it either.

Comment 6 Joseph Pecoraro 2014-01-15 11:34:08 PST

Comment on attachment 221255 [details]
Patch with removed files

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

> Source/WebInspectorUI/UserInterface/CSSSelectorProfileType.js:-61
> -        return WebInspector.UIString("CSS selector profiles show how long the selector matching has taken in total and how many times a certain selector has matched DOM elements (the results are approximate due to matching algorithm optimizations.)");

Nit: This patch removes localized strings but does not update localizedStrings.js.

To automatically update the localized strings you can run:

    shell> ./Tools/Scripts/extract-localizable-js-strings ./Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js ./Source/WebInspectorUI/UserInterface

There is a basic script you can run, something like ./Tools/Scripts/update-localizable-strings, but the command above just targets WebInspectorUI strings and takes ~1 second.

I'll update the strings now.

Good job on this removal! I think you covered everything!

Comment 7 Joseph Pecoraro 2014-01-15 11:35:39 PST

(In reply to comment #6)
> I'll update the strings now.

Actually, this hasn't landed yet! So there is time for you to update the strings =)

Comment 8 Andreas Kling 2014-01-15 12:05:14 PST

Committed r162084: <http://trac.webkit.org/changeset/162084>