Bug 96405 - Web Inspector: NMI move String* instrumentation to wtf
Summary: Web Inspector: NMI move String* instrumentation to wtf
Status: RESOLVED FIXED
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: 96487
Blocks: 96367
  Show dependency treegraph
 
Reported: 2012-09-11 10:19 PDT by Ilya Tikhonovsky
Modified: 2012-09-12 05:49 PDT (History)
14 users (show)

See Also:


Attachments
Patch (13.02 KB, patch)
2012-09-11 10:25 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (17.09 KB, patch)
2012-09-12 01:46 PDT, Ilya Tikhonovsky
yurys: review+
webkit.review.bot: commit-queue-
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-09-11 10:19:57 PDT
It is hard to upstream instrumentation traits for string classes.
It is much simple to instrument string classes directly.
Also this instrumentation is solving the problem with SubStrings.
Comment 1 Ilya Tikhonovsky 2012-09-11 10:25:18 PDT
Created attachment 163398 [details]
Patch
Comment 2 Yury Semikhatsky 2012-09-12 01:34:40 PDT
Comment on attachment 163398 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.h:723
> +        // count size used by internal buffer but skip strings that were constructed from literals.

count -> Count

> Source/WTF/wtf/text/StringImpl.h:731
> +        typename MemoryObjectInfo::ClassInfo info(memoryObjectInfo, this, 0, selfSize);

You should be consistent in using either constant MemoryObjectType::Unknown or 0 everywhere.

> Source/WTF/wtf/text/StringImpl.h:735
> +        else if (m_hashAndFlags & s_hashFlagHas16BitShadow) // Substrings never have it's own shadow.

typo: Substring never has its own shadow.
Comment 3 Ilya Tikhonovsky 2012-09-12 01:46:50 PDT
Created attachment 163546 [details]
Patch
Comment 4 Yury Semikhatsky 2012-09-12 01:52:57 PDT
Comment on attachment 163546 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.h:735
> +        else if (m_hashAndFlags & s_hashFlagHas16BitShadow) // Substring never has it's own shadow.

it's -> its
Comment 5 Ilya Tikhonovsky 2012-09-12 01:57:46 PDT
(In reply to comment #2)
> (From update of attachment 163398 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163398&action=review
> 
> > Source/WTF/wtf/text/StringImpl.h:723
> > +        // count size used by internal buffer but skip strings that were constructed from literals.
> 
> count -> Count

Done

> 
> > Source/WTF/wtf/text/StringImpl.h:731
> > +        typename MemoryObjectInfo::ClassInfo info(memoryObjectInfo, this, 0, selfSize);
> 
> You should be consistent in using either constant MemoryObjectType::Unknown or 0 everywhere.

Ok I think we can use 0 everywhere otherwise I need to have a separate header file for generic memory types and cpp file for the value.

> 
> > Source/WTF/wtf/text/StringImpl.h:735
> > +        else if (m_hashAndFlags & s_hashFlagHas16BitShadow) // Substrings never have it's own shadow.
> 
> typo: Substring never has its own shadow.

Done.
Comment 6 Ilya Tikhonovsky 2012-09-12 02:13:06 PDT
Committed r128279: <http://trac.webkit.org/changeset/128279>
Comment 7 WebKit Review Bot 2012-09-12 02:39:55 PDT
Re-opened since this is blocked by 96487
Comment 8 WebKit Review Bot 2012-09-12 05:26:38 PDT
Comment on attachment 163546 [details]
Patch

Attachment 163546 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13811919

New failing tests:
MemoryInstrumentationTest.visitFirstMemberInNonVirtualClass
MemoryInstrumentationTest.dataRef
MemoryInstrumentationTest.sizeOf
inspector/profiler/memory-instrumentation-cached-images.html
MemoryInstrumentationTest.ptrVsRef
MemoryInstrumentationTest.refPtr
MemoryInstrumentationTest.ownerTypePropagation
MemoryInstrumentationTest.ownPtr
MemoryInstrumentationTest.ownPtrNotInstrumented
MemoryInstrumentationTest.visitStrings
Comment 9 Ilya Tikhonovsky 2012-09-12 05:49:52 PDT
Committed r128300: <http://trac.webkit.org/changeset/128300>