EOM
Created attachment 166361 [details] Patch
Comment on attachment 166361 [details] Patch Attachment 166361 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14084213
Comment on attachment 166361 [details] Patch Attachment 166361 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14091212
Comment on attachment 166361 [details] Patch Attachment 166361 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14078272
Comment on attachment 166361 [details] Patch Attachment 166361 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14074243
Comment on attachment 166361 [details] Patch Attachment 166361 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14079219
Created attachment 166362 [details] Patch
Created attachment 166364 [details] fix for KURL
Comment on attachment 166364 [details] fix for KURL Attachment 166364 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14088219
Comment on attachment 166364 [details] fix for KURL Attachment 166364 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14081247
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
Comment on attachment 166364 [details] fix for KURL Attachment 166364 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14089246
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
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() ?
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.
Created attachment 166437 [details] comments addressed
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.
Created attachment 166470 [details] comments addressed
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 ?
Created attachment 166477 [details] usesInternalBuffer was renamed to hasInternalBuffer. hasOwnedBuffer was added.
Created attachment 166488 [details] rebaselined.
Committed r130129: <http://trac.webkit.org/changeset/130129>
Re-opened since this is blocked by bug 98125
Committed r130144: <http://trac.webkit.org/changeset/130144>
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?
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.
> 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).
(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.