Bug 98500 - Web Inspector: NMI fix String instrumentation the way it was discussed in WK97964
Summary: Web Inspector: NMI fix String instrumentation the way it was discussed in WK9...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL:
Keywords:
Depends on: 97964
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-05 01:58 PDT by Ilya Tikhonovsky
Modified: 2014-12-12 13:43 PST (History)
14 users (show)

See Also:


Attachments
Patch (8.06 KB, patch)
2012-10-05 02:08 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2012-10-05 01:58:57 PDT
Current instrumentation incorrectly covers the case when StringImpl is created via StringImpl::createWithTerminatingNullCharacter()
Looks like the only way to detect the strings that were created from literals is to compare the addresses of buffer and stringImpl + 1.
Comment 1 Ilya Tikhonovsky 2012-10-05 02:08:01 PDT
Created attachment 167288 [details]
Patch
Comment 2 Benjamin Poulain 2012-10-05 12:29:33 PDT
Comment on attachment 167288 [details]
Patch

I can't help but wonder why MemoryInstrumentationTest.cpp is in Chromium instead of being cross-port in WebKitTestAPI.

Otherwise the change looks good.
Comment 3 Benjamin Poulain 2012-10-05 12:30:29 PDT
Why isn't memory instrumentation behind a feature flag? It looks like an experimental feature to me.
Comment 4 WebKit Review Bot 2012-10-06 02:26:19 PDT
Comment on attachment 167288 [details]
Patch

Clearing flags on attachment: 167288

Committed r130581: <http://trac.webkit.org/changeset/130581>
Comment 5 WebKit Review Bot 2012-10-06 02:26:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Ilya Tikhonovsky 2012-10-11 00:12:00 PDT
(In reply to comment #3)
> Why isn't memory instrumentation behind a feature flag? It looks like an experimental feature to me.

We decided to do the work without a flag because the feature doesn't affects UI, the rendering and the performance.  It is quite challenging task by itself because we need to instrument a lot of classes for the better coverage and we would have additional work in case of being behind a flag because WebKit is constantly moving forward.

Additional burden on the developers is small because the body of reportMemoryUsage member function for a typical class is very simple in almost all the cases and requires the attention only in the case of removing a member variable.

We monitor the quality of the instrumentation with help of tcmalloc profiler.
Also we will use clang plugin for the coverage check on the later stage.
Comment 7 Ilya Tikhonovsky 2012-10-11 00:55:34 PDT
(In reply to comment #2)
> (From update of attachment 167288 [details])
> I can't help but wonder why MemoryInstrumentationTest.cpp is in Chromium instead of being cross-port in WebKitTestAPI.
> 
> Otherwise the change looks good.

Good point. I'll move it to to TestWebKitAPI.
Comment 8 Brian Burg 2014-12-12 13:40:51 PST
Closing as invalid, as this bug pertains to the old inspector UI and/or its tests.
Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.