Bug 95807 – ENH: Add Logging to StringImpl to track String Types

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

As an aid to converting more WebKit code to process 8 bit strings, it would be good to have statistics showing various statistics like the number of strings, how many are 8 versus 16 bits, how many bytes they use and how many 8 bit strings are up converted to 16 bits.

[Michael Saboff]

Created attachment 162328 [details]
Patch

[Benjamin Poulain]

I am not super convinced we should have that in the code.

I agree we need better instrumentation, but I have found in my owns that the origins of strings is important to be able to work.

I also think this is gonna become dead code once you move on to another part of WebCore.

What I would like for instrumentations is:
-log all the creation and destruction of StringImpl with DTrace
-log the pointer so we can track individual strings
-parse everything with python to a SQLLite database
-->make query on the data

What do you think?

[Michael Saboff]

(In reply to comment #2)
> I am not super convinced we should have that in the code.
> 
> I agree we need better instrumentation, but I have found in my owns that the origins of strings is important to be able to work.
> 
> I also think this is gonna become dead code once you move on to another part of WebCore.
> 
> What I would like for instrumentations is:
> -log all the creation and destruction of StringImpl with DTrace
> -log the pointer so we can track individual strings
> -parse everything with python to a SQLLite database
> -->make query on the data
> 
> What do you think?

This code provides a dashboard style of statistics.  I have already used it side by side with a before / after change running Safari.  You can quickly see deltas.

What you describe would allow more introspection but with greater effort to get the information.  As I see it, this changes tells you how much work you have to do to remove 16 bit strings, while your suggestion helps find the problem once you know there is an issue.  This change can provide some help with that as well, as you can run under the debugger with breakpoints in StringStats::add16BitString or StringStats::addUpconvertedString.

As far as becoming dead code, the changes are localized to two files and easily removed if/when we get to that point.

[Benjamin Poulain]

Comment on attachment 162328 [details]
Patch

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

I have nothing against this, but I don't get why this should be in WebKit instead of just a "tool commit" on your local tree.


> Source/WTF/ChangeLog:11
> +        strings as well as the number of 8 bit strings up converted to 16 bits.  The number of characrters
> +        for each type is also accumulated.  These statistics are output via DataLog every 5000
> +        calls to StringImpl destructor.  The 5000 can be adjusted via s_printStringStatsFrequency.

Two spaces after each period for some reason...

> Source/WTF/wtf/text/StringImpl.h:234
> +
> +        STRING_STATS_ADD_8BIT_STRING(m_length);

Reporting m_length here is not always accurate, it is a own buffer.

> Source/WTF/wtf/text/StringImpl.h:249
> +
> +        STRING_STATS_ADD_8BIT_STRING(m_length);

Reporting m_length  here is incorrect.
String from literal do not allocate memory for the data.

> Source/WTF/wtf/text/StringImpl.h:263
> +
> +        STRING_STATS_ADD_16BIT_STRING(m_length);

Same comment about buffer owned.

> Source/WTF/wtf/text/StringImpl.h:341
> +
> +        STRING_STATS_ADD_16BIT_STRING(m_length);

This code is using a buffer ownership type dependency. It does not allocate m_length memory. This is similar to BufferOwned.

[Sam Weinig]

Comment on attachment 162328 [details]
Patch

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

>> Source/WTF/ChangeLog:11
>> +        calls to StringImpl destructor.  The 5000 can be adjusted via s_printStringStatsFrequency.
> 
> Two spaces after each period for some reason...

Some habits are hard to drop.

> Source/WTF/wtf/text/StringImpl.cpp:33
> +#ifdef STRING_STATS
> +#include <wtf/DataLog.h>
> +#endif

It reads cleaner if you put the #ifdef #includes at the bottom.

[Michael Saboff]

(In reply to comment #4)
> (From update of attachment 162328 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162328&action=review
> 
> I have nothing against this, but I don't get why this should be in WebKit instead of just a "tool commit" on your local tree.
> 
> 
> > Source/WTF/ChangeLog:11
> > +        strings as well as the number of 8 bit strings up converted to 16 bits.  The number of characrters
> > +        for each type is also accumulated.  These statistics are output via DataLog every 5000
> > +        calls to StringImpl destructor.  The 5000 can be adjusted via s_printStringStatsFrequency.
> 
> Two spaces after each period for some reason...

Two spaces after a period for a fixed pitch font (like a ChangeLog) was drilled into me many years ago and she learned it probably in the 40's or 50's (that's 1940-1950's). ;-) It's hard to unlearn muscle memory as Sam points out.

> > Source/WTF/wtf/text/StringImpl.h:234
> > +
> > +        STRING_STATS_ADD_8BIT_STRING(m_length);
> 
> Reporting m_length here is not always accurate, it is a own buffer.

Reporting own buffer lengths is a debatable point. Much of this comes from StringBuilder or other build-as-you-go constructs. I actually do want to know how much data is stored in strings for this type.

> > Source/WTF/wtf/text/StringImpl.h:249
> > +
> > +        STRING_STATS_ADD_8BIT_STRING(m_length);
> 
> Reporting m_length  here is incorrect.
> String from literal do not allocate memory for the data.

Changed this to 0.

> > Source/WTF/wtf/text/StringImpl.h:263
> > +
> > +        STRING_STATS_ADD_16BIT_STRING(m_length);
> 
> Same comment about buffer owned.
> 
> > Source/WTF/wtf/text/StringImpl.h:341
> > +
> > +        STRING_STATS_ADD_16BIT_STRING(m_length);
> 
> This code is using a buffer ownership type dependency. It does not allocate m_length memory. This is similar to BufferOwned.

I can post an updated patch.

The real issue is whether or not this change is generally useful. I find it useful and think others will as well. I do think that its usefulness extends beyond the current make as much code 8 bit aware as possible since it can be used to find regressions.  If the consensus is that it is code pollution, I'll just keep my own copy.

[Benjamin Poulain]

Comment on attachment 162328 [details]
Patch

Ok, I am convinced :)

r+ with the updates.

[Michael Saboff]

Committed r127799: <http://trac.webkit.org/changeset/127799>

[Michael Saboff]

Additional commit to fix building with STRING_STATS enabled.

Committed r127805: <http://trac.webkit.org/changeset/127805>