Bug 95739

Summary:

Web Inspector: NMI: replace ObjectType enum with static const char* string identifiers.

Product:

WebKit

Reporter:

Ilya Tikhonovsky <loislo>

Component:

Web Inspector (Deprecated)

Assignee:

Ilya Tikhonovsky <loislo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alph, apavlov, bweinstein, dglazkov, jamesr, joepeck, keishi, loislo, peter+ews, pfeldman, pmuellr, rik, timothy, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	yurys: review+

Description Ilya Tikhonovsky 2012-09-04 05:44:05 PDT

When we go deeper into different parts of browser like skia, chromium itself etc.
we can't use a single enum for all reported object types.
We need a code that allows us to register object types on the fly.
On the next step we will make string identifiers hierarchical like DOM, DOM.Bindings, DOM.Other etc.

Comment 1 Ilya Tikhonovsky 2012-09-04 05:51:45 PDT

Created attachment 162024 [details]
Patch

Comment 2 Build Bot 2012-09-04 06:23:59 PDT

Comment on attachment 162024 [details]
Patch

Attachment 162024 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13743484

Comment 3 Yury Semikhatsky 2012-09-04 06:53:36 PDT

Comment on attachment 162024 [details]
Patch

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

> Source/WebCore/inspector/MemoryInstrumentationImpl.cpp:40
> +Vector<String> WebCoreMemoryTypesImpl::registerObjectTypes(unsigned firstFreeId)

I'd rather use strings instead of numbers for identifying object type. We could switch to the scheme with the type registration later if there is a noticeable performance impact. WDYT?

Comment 4 Early Warning System Bot 2012-09-04 07:11:53 PDT

Comment on attachment 162024 [details]
Patch

Attachment 162024 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13757216

Comment 5 Early Warning System Bot 2012-09-04 07:43:12 PDT

Comment on attachment 162024 [details]
Patch

Attachment 162024 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13757228

Comment 6 Peter Beverloo (cr-android ews) 2012-09-04 07:45:58 PDT

Comment on attachment 162024 [details]
Patch

Attachment 162024 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13745476

Comment 7 Ilya Tikhonovsky 2012-09-04 08:12:14 PDT

(In reply to comment #3)
> (From update of attachment 162024 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162024&action=review
> 
> > Source/WebCore/inspector/MemoryInstrumentationImpl.cpp:40
> > +Vector<String> WebCoreMemoryTypesImpl::registerObjectTypes(unsigned firstFreeId)
> 
> I'd rather use strings instead of numbers for identifying object type. We could switch to the scheme with the type registration later if there is a noticeable performance impact. WDYT?

Looks like the version which uses char* as ObjectType is 30% slower than current implementation. 990ms vs 750ms on 10m insertions with disabled visited check.
I think we could use this approach as the solution because we will have simpler business logic.

Comment 8 Ilya Tikhonovsky 2012-09-04 08:21:38 PDT

Created attachment 162046 [details]
Patch

Comment 9 Yury Semikhatsky 2012-09-05 02:00:30 PDT

Comment on attachment 162046 [details]
Patch

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

> Source/WebCore/dom/MemoryInstrumentation.h:53
> +    static const char* Other;

const char* -> ObjectType

Comment 10 Ilya Tikhonovsky 2012-09-05 02:19:28 PDT

Committed r127571: <http://trac.webkit.org/changeset/127571>

Comment 11 James Robinson 2012-09-05 11:25:42 PDT

The "speedTest" unit test takes over 20 seconds to run on my extremely fast desktop.  That's waaaaaaaaaaaaaaaaay too long for a unit test.