Bug 97964 – Web Inspector: NMI make String* instrumentation non intrusive

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

EOM

[Ilya Tikhonovsky]

Created attachment 166361 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 166361 [details]
Patch

Attachment 166361 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14084213

[Early Warning System Bot]

Comment on attachment 166361 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 166361 [details]
Patch

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

[Build Bot]

Comment on attachment 166361 [details]
Patch

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

[Build Bot]

Comment on attachment 166361 [details]
Patch

Attachment 166361 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14079219

[Ilya Tikhonovsky]

Created attachment 166362 [details]
Patch

[Ilya Tikhonovsky]

Created attachment 166364 [details]
fix for KURL

[Early Warning System Bot]

Comment on attachment 166364 [details]
fix for KURL

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

[Gyuyoung Kim]

Comment on attachment 166364 [details]
fix for KURL

Attachment 166364 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14081247

[Early Warning System Bot]

Comment on attachment 166364 [details]
fix for KURL

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

[Gyuyoung Kim]

Comment on attachment 166364 [details]
fix for KURL

Attachment 166364 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14089246

[Build Bot]

Comment on attachment 166364 [details]
fix for KURL

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

New failing tests:
svg/animations/smil-leak-dynamically-added-element-instances.svg

[Yury Semikhatsky]

Comment on attachment 166364 [details]
fix for KURL

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

> Source/WTF/wtf/MemoryInstrumentation.h:34
> +#include <stdio.h>

Remove this.

> Source/WTF/wtf/MemoryInstrumentation.h:340
> +void reportMemoryUsage(const String* const&, MemoryObjectInfo*);

Please update comment at line 335

> Source/WTF/wtf/text/StringImpl.h:456
> +    bool usesInternalBuffer() const { return bufferOwnership() == BufferInternal && !hasTerminatingNullCharacter(); }

Why do you need !hasTerminatingNullCharacter() ?

[Yury Semikhatsky]

Comment on attachment 166364 [details]
fix for KURL

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

> Source/WTF/wtf/MemoryInstrumentationString.h:46
> +    if (stringImpl->usesInternalBuffer() && !stringImpl->hasTerminatingNullCharacter())

It doesn't seem to work for BufferOwned type of character buffer ownership(everything created with StringImpl::adopt). Please fix that and provide a test for that case.

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:277
>      }

Pleas add a test for substring and for a regular string without shadow.

[Ilya Tikhonovsky]

Created attachment 166437 [details]
comments addressed

[Yury Semikhatsky]

Comment on attachment 166437 [details]
comments addressed

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

> Source/WTF/wtf/MemoryInstrumentationString.h:46
> +    if (stringImpl->usesInternalBuffer() && !stringImpl->hasTerminatingNullCharacter())

BufferOwned case is still missing as well as test for that case, r- for that.

[Ilya Tikhonovsky]

Created attachment 166470 [details]
comments addressed

[Yury Semikhatsky]

Comment on attachment 166470 [details]
comments addressed

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

> Source/WTF/wtf/text/StringImpl.h:456
> +    bool usesInternalBuffer() const { return bufferOwnership() == BufferInternal && !hasTerminatingNullCharacter(); }

hasInternalBuffer ?

[Ilya Tikhonovsky]

Created attachment 166477 [details]
usesInternalBuffer was renamed to hasInternalBuffer. hasOwnedBuffer was added.

[Ilya Tikhonovsky]

Created attachment 166488 [details]
rebaselined.

[Ilya Tikhonovsky]

Committed r130129: <http://trac.webkit.org/changeset/130129>

[WebKit Review Bot]

Re-opened since this is blocked by bug 98125

[Ilya Tikhonovsky]

Committed r130144: <http://trac.webkit.org/changeset/130144>

[Benjamin Poulain]

Comment on attachment 166488 [details]
rebaselined.

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

> Source/WTF/wtf/MemoryInstrumentationString.h:47
> +    if (stringImpl->hasInternalBuffer())

This looks incorrect. StringImpl::createWithTerminatingNullCharacter() create strings with the same conditions as hasInternalBuffer().

> Source/WTF/wtf/MemoryInstrumentationString.h:59
> +                info.addRawBuffer(stringImpl->characters16(), stringImpl->length() * 2);

* 2, really?

[Ilya Tikhonovsky]

Comment on attachment 166488 [details]
rebaselined.

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

>> Source/WTF/wtf/MemoryInstrumentationString.h:47
>> +    if (stringImpl->hasInternalBuffer())
> 
> This looks incorrect. StringImpl::createWithTerminatingNullCharacter() create strings with the same conditions as hasInternalBuffer().

Good point. Looks like in these two cases, construct from literal and create with terminating null character, we can't decide is the string uses internal buffer or not, and can't report correct physical size of the memory object.
The only way is to check that the buffer address equal to stringimpl address + sizeof(StringImpl).

>> Source/WTF/wtf/MemoryInstrumentationString.h:59
>> +                info.addRawBuffer(stringImpl->characters16(), stringImpl->length() * 2);
> 
> * 2, really?

why not?  we are calculating the size of buffer in bytes and this is 16-bit per symbol string.

[Benjamin Poulain]

> The only way is to check that the buffer address equal to stringimpl address + sizeof(StringImpl).

Exactly (and that's cheaper to compute too) :)

> >> +                info.addRawBuffer(stringImpl->characters16(), stringImpl->length() * 2);
> > 
> > * 2, really?
> 
> why not?  we are calculating the size of buffer in bytes and this is 16-bit per symbol string.

Typically in WebKit, we don't use magic numbers. In this case, one would use sizeof(UChar).

[Ilya Tikhonovsky]

(In reply to comment #27)
> > The only way is to check that the buffer address equal to stringimpl address + sizeof(StringImpl).
> 
> Exactly (and that's cheaper to compute too) :)
> 
> > >> +                info.addRawBuffer(stringImpl->characters16(), stringImpl->length() * 2);
> > > 
> > > * 2, really?
> > 
> > why not?  we are calculating the size of buffer in bytes and this is 16-bit per symbol string.
> 
> Typically in WebKit, we don't use magic numbers. In this case, one would use sizeof(UChar).

yep. my fault.