Bug 66851 – Fix CSSPrimitiveValue::cssText() to use StringBuilder

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: CSS

[Oliver Varga]

Replace the string concatenation (+, +=, and Vector<Uchar> append()) to the much faster StringBuilder append().
http://www.mail-archive.com/[e-mail]/msg16022.html

[Oliver Varga]

Created attachment 104982 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

[Andreas Kling]

Comment on attachment 104982 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

> Source/WebCore/ChangeLog:14
> +        According to the tests youtube, ebay,
> +        baidu and wordpress loading is faster about 5-8%.

5-8%? Seriously? That sounds way too good to be true, how are you testing this exactly?

[Darin Adler]

Comment on attachment 104982 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

Thanks for tackling this. I see no reason to overload the += operator, though. Lets use append.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:57
> +    inline void operator+=(const String& string)

Why is it critical to use += instead of an append function? Just to keep the editing down?

> Source/WebCore/css/CSSPrimitiveValue.cpp:646
> +            text += formatNumber(m_value.num);
> +            text += "em";

I think these would read better as append function calls.

[Zoltan Herczeg]

I suggested Oliver to keep the += since for me it is easier to read. Sorry Oliver for the extra work.

[Oliver Varga]

(In reply to comment #2)
> (From update of attachment 104982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104982&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        According to the tests youtube, ebay,
> > +        baidu and wordpress loading is faster about 5-8%.
> 
> 5-8%? Seriously? That sounds way too good to be true, how are you testing this exactly?

I tested our internal testing system, called "methanol benchmarking".
It is loading webpages many times and it is measured.

[Oliver Varga]

Created attachment 105141 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

[Oliver Varga]

(In reply to comment #2)
> (From update of attachment 104982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104982&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        According to the tests youtube, ebay,
> > +        baidu and wordpress loading is faster about 5-8%.
> 
> 5-8%? Seriously? That sounds way too good to be true, how are you testing this exactly?

You are right, the 5-8% is too good to be true.
I performed the tests again, and now generated a minimal performance tweak.

[Oliver Varga]

Created attachment 105144 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

[Darin Adler]

Comment on attachment 105144 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

My comments include many ideas that are ways to do better in the future. The one thing you should definitely fix before landing is the append(c) thing. Please consider the others too, though. We don’t want any changes that actually make things slower.

> Source/WebCore/css/CSSPrimitiveValue.cpp:638
> +            text.append(formatNumber(m_value.num));

Suggestion for further improvement:

There’s a significant performance gain available if we can come up with a way to do these without creating a string and then appending it. We need an equivalent of formatNumber that works directly with the StringBuilder, or some way that does not involve memory allocation other than what StringBuilder does.

> Source/WebCore/css/CSSPrimitiveValue.cpp:716
> -            text = quoteCSSStringIfNeeded(m_value.string);
> +            text.append(quoteCSSStringIfNeeded(m_value.string));

Similarly, for the quoted string case it's much more efficient to do the quoting without allocating an entire new string.

> Source/WebCore/css/CSSPrimitiveValue.cpp:732
>              DEFINE_STATIC_LOCAL(const String, attrParen, ("attr("));

There's no value in having this pre-allocated in a String. It can just be an append of a const char*.

> Source/WebCore/css/CSSPrimitiveValue.cpp:755
>              DEFINE_STATIC_LOCAL(const String, rectParen, ("rect("));

There's no value in having this pre-allocated in a String. It can just be an append of a const char*.

> Source/WebCore/css/CSSPrimitiveValue.cpp:-737
> -            Vector<UChar> result;
> -            result.reserveInitialCapacity(32);

Are you sure the StringBuilder is more efficient than the Vector<UChar> was?

> Source/WebCore/css/CSSPrimitiveValue.cpp:762
> +            result.append(rectVal->top()->cssText());

This is another opportunity to be much more efficient. If we had a function that appended the CSS text to a StringBuilder without ever creating a string, we could probably save many extra string allocations here.

> Source/WebCore/css/CSSPrimitiveValue.cpp:794
> -            appendNumber(result, static_cast<unsigned char>(color.red()));
> -            append(result, commaSpace);
> +            result.append(String::number(color.red()));

Again, making a string out of the number just to put it in the string builder is quite inefficient. It’s a step backwards to go from the appendNumber function, which can and has been optimized, to an implementation that explicitly allocates a string each time.

> Source/WebCore/css/CSSPrimitiveValue.cpp:853
>              char c = static_cast<char>(m_value.ident);
> -            text = String(&c, 1U);
> +            text.append(String(&c, 1U));

This is wrong. It can just be append(c) or even append(static_cast<char>(m_value.ident)). Explicitly creating a string makes us dow double allocation.

> Source/WebCore/css/CSSPrimitiveValue.cpp:867
> -    cssTextCache().set(this, text);
> +    cssTextCache().set(this, text.toString());
>      m_hasCachedCSSText = true;
> -    return text;
> +    return text.toString();

Calling toString() twice seems a bit inelegant. I would put the string into a local variable instead.

[Oliver Varga]

(In reply to comment #9)
> (From update of attachment 105144 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105144&action=review
> 
> My comments include many ideas that are ways to do better in the future. The one thing you should definitely fix before landing is the append(c) thing. Please consider the others too, though. We don’t want any changes that actually make things slower.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:638
> > +            text.append(formatNumber(m_value.num));
> 
> Suggestion for further improvement:
> 
> There’s a significant performance gain available if we can come up with a way to do these without creating a string and then appending it. We need an equivalent of formatNumber that works directly with the StringBuilder, or some way that does not involve memory allocation other than what StringBuilder does.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:716
> > -            text = quoteCSSStringIfNeeded(m_value.string);
> > +            text.append(quoteCSSStringIfNeeded(m_value.string));
> 
> Similarly, for the quoted string case it's much more efficient to do the quoting without allocating an entire new string.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:732
> >              DEFINE_STATIC_LOCAL(const String, attrParen, ("attr("));
> 
> There's no value in having this pre-allocated in a String. It can just be an append of a const char*.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:755
> >              DEFINE_STATIC_LOCAL(const String, rectParen, ("rect("));
> 
> There's no value in having this pre-allocated in a String. It can just be an append of a const char*.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:-737
> > -            Vector<UChar> result;
> > -            result.reserveInitialCapacity(32);
> 
> Are you sure the StringBuilder is more efficient than the Vector<UChar> was?
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:762
> > +            result.append(rectVal->top()->cssText());
> 
> This is another opportunity to be much more efficient. If we had a function that appended the CSS text to a StringBuilder without ever creating a string, we could probably save many extra string allocations here.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:794
> > -            appendNumber(result, static_cast<unsigned char>(color.red()));
> > -            append(result, commaSpace);
> > +            result.append(String::number(color.red()));
> 
> Again, making a string out of the number just to put it in the string builder is quite inefficient. It’s a step backwards to go from the appendNumber function, which can and has been optimized, to an implementation that explicitly allocates a string each time.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:853
> >              char c = static_cast<char>(m_value.ident);
> > -            text = String(&c, 1U);
> > +            text.append(String(&c, 1U));
> 
> This is wrong. It can just be append(c) or even append(static_cast<char>(m_value.ident)). Explicitly creating a string makes us dow double allocation.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:867
> > -    cssTextCache().set(this, text);
> > +    cssTextCache().set(this, text.toString());
> >      m_hasCachedCSSText = true;
> > -    return text;
> > +    return text.toString();
> 
> Calling toString() twice seems a bit inelegant. I would put the string into a local variable instead.

Sorry for the inattention, now I'm correcting.
And thank you for the guidance. I will considered your suggestions.

[Oliver Varga]

Created attachment 105339 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

[Oliver Varga]

Created attachment 107345 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

[WebKit Review Bot]

Attachment 107345 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/WebCore/css/CSSPrimitiveValue.cpp:894:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/wtf/text/StringBuilder.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/wtf/text/StringBuilder.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/wtf/text/StringBuilder.h:108:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/wtf/text/StringBuilder.h:144:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/text/StringBuilder.h:150:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/wtf/text/StringBuilder.h:166:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 7 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Varga]

Created attachment 107347 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

[WebKit Review Bot]

Comment on attachment 107347 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

New failing tests:
editing/execCommand/query-command-value-background-color.html
svg/animations/animate-color-rgba-calcMode-discrete.html

[Darin Adler]

Comment on attachment 107347 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

> Source/JavaScriptCore/wtf/text/StringBuilder.h:105
> +        char* number = new char[absoluteExp + sizeOfsignificand + 3];

This is an inefficient way to allocate memory, and the wrong way to go for this function. The point of such functions is to be fast and doing a malloc makes them slow. An efficient way to allocate memory is:

    Vector<char, 32> number(absoluteExp + sizeOfsignificand + 3);

This will use the stack for the buffer unless the buffer has to be larger than 32 bytes.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:158
> +        char* number = new char[digitsOfNumber + 1];

Ditto.

[Oliver Varga]

Created attachment 111022 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

[WebKit Review Bot]

Comment on attachment 111022 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

[Darin Adler]

Comment on attachment 111022 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

> Source/JavaScriptCore/wtf/text/StringBuilder.h:135
> +    void append(double d, int precision = 6)

This giant function should not be inlined in a header.

[Oliver Varga]

Created attachment 111257 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

[WebKit Review Bot]

Attachment 111257 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   293b636..9b6f4d5  master     -> origin/master
	M	Tools/BuildSlaveSupport/build.webkit.org-config/config.json
	M	Tools/ChangeLog
r97614 = 9b6f4d5fe02791ab160cc89f3585e1ae198694bd (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Varga]

Created attachment 111441 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

[Oliver Varga]

Created attachment 111570 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

[WebKit Review Bot]

Attachment 111570 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/text/StringBuilder.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Varga]

Created attachment 111571 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

[Oliver Varga]

Created attachment 111955 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

I correct the previous mistake I found here: http://queues.webkit.org/results/10121269
After that I didn't found any information in connection with the Build Failed on win EWS.
I try to upload again the previous patch.

[Darin Adler]

Comment on attachment 111955 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

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

If you want a patch to be reviewed, you need to set review+ on it.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:82
> +    static inline unsigned getPrecision(int m_exponent, unsigned m_precision, unsigned digitsBeforeDecimalPoint)

The inline keyword here is not needed.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:108
> +    {
> +            // #shift
> +            int p = 1;
> +            while (num[p+1] != '\0' && p <= shift && num[1] != '\0') {
> +                    num[ p ] = num[p+1];
> +                    num[p+1] = '.';
> +                    p++;
> +            }

This function is indented 8 rather than 4 space tab stops.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:136
> +    void append(double d)

This large function should not be inlined in a header.

[Oliver Varga]

Created attachment 111969 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

I corrected the indentation, and deleted the inline modifier.

[Andreas Kling]

Comment on attachment 111969 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

You need to move the huge functions in StringBuilder.h to a cpp file to prevent them from being inlined at least once per compilation unit that calls them.

[Oliver Varga]

Created attachment 112126 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

The huge functions moved to the StringBuilder.cpp

[Kent Tamura]

Comment on attachment 112126 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

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

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:180
> +       length += -m_exponent - 1;
> +       length += m_precision;
> +       return length;

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:184
> +       return 0;

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:198
> +    while (num[p+1] != '\0' && p <= shift && num[1] != '\0') {
> +         num[ p ] = num[p+1];
> +         num[p+1] = '.';
> +         p++;

5 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:206
> +    while (p <= shift) {
> +         num[ p ] = '0';
> +         num[p+1] = '.';
> +         num[p+2] = 0;
> +         p++;

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:212
> +    for (int i = length - 1; i >= p && (num[i] == '0' || num[i] == '.'); --i)
> +         num[i]=0;

5 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:216
> +    if (num[0] == '0' && num[1] == '0')
> +         return 2;

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:219
> +    if (num[0] == '0' && num[1] != '.')
> +         return 1;

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:222
> +    if (num[0] != '0')
> +       return 0;

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:238
> +    if (d < 0) {
> +       isNegative = true;
> +       d = -d;
> +       StringBuilder::append("-", 1);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:276
> +    if (!precision) {
> +       if (m_exponent < 0) {
> +         m_significand[2] = m_significand[1];

3 space indentation,
and 2 space indentation.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:284
> +       for (; i < digitsBeforeDecimalPoint && i < sizeOfsignificand; ++i) {
> +         *pointer = m_significand[i];

2 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:289
> +       if (m_significand[i] > '4') {
> +         toShif = true;

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:300
> +         if (m_significand[0] == '0') {
> +            maxLimit--;

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:305
> +    } else
> +       jump = true;

3 space indentation.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:310
> +    if (jump) {
> +       for (int i = 0; i < numberOfZeros; i++) {
> +          if (i == 1 && !dotAlreadyWritten) {
> +             *pointer = dot;

3 space indentations

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:319
> +          if (maxLimit == precision) {
> +             if (numberOfZeros > precision + 1 || (numberOfZeros == (precision + 1) && m_significand[0] < '5')) {
> +               pointer = &number[1];

3 space indentation and 2 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:324
> +               if (toShif)
> +                  from = shifting(&number[0], valueOfShift);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:330
> +             if (numberOfZeros == precision + 1 && m_significand[0] > '4') {
> +               pointer[-1] = '1';

2 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:334
> +               if (toShif)
> +                  from = shifting(&number[0], valueOfShift);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:343
> +       for (int i = 0; i < sizeOfsignificand; i++) {
> +          if (i == placeOfDot + 1 && sizeOfsignificand != i && !dotAlreadyWritten) {
> +             *pointer = dot;

3 space indentations

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:351
> +          if (maxLimit == precision) {
> +             if (i + 1 < sizeOfMSignificand && m_significand[i + 1] < '5') {
> +               while (*pointer == '0' && dotAlreadyWritten)

3 space indentation,
and 2 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:354
> +                if (*pointer == dot)
> +                   *pointer = '\0';

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:358
> +                if (toShif)
> +                   from = shifting(&number[0], valueOfShift);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:365
> +              if (m_significand[i] == '9') {
> +                while (*pointer == '9' && *pointer != dot)
> +                  pointer--;

2 space indentations

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:368
> +                if (*pointer == dot)
> +                     pointer--;

5 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:370
> +                 pointer[1] = '\0';

Unnecessary 1 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:373
> +                if (toShif)
> +                   from = shifting(&number[0], valueOfShift);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:383
> +              if (toShif)
> +                from = shifting(&number[0], valueOfShift);

2 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:393
> +    for (int i = 0; i < (m_exponent - sizeOfsignificand) + 1; i++) {
> +       *pointer = zero;
> +       pointer++;

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:399
> +    if (toShif)
> +       from = shifting(&number[0], valueOfShift);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:407
> +    if (n < 0) {
> +       StringBuilder::append("-", 1);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:412
> +    if (!(n / 10)) {
> +       char c = static_cast<char>(n + '0');

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:424
> +    do {
> +       *pointer = (n % 10) + '0';

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.h:82
> +    static unsigned getPrecision(int m_exponent, unsigned m_precision, unsigned digitsBeforeDecimalPoint);

Do not prepend 'm_' for argument names.

[Darin Adler]

Comment on attachment 112126 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

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

> Source/JavaScriptCore/wtf/text/StringBuilder.h:31
> +#include <math.h>
> +#include <wtf/MathExtras.h>
> +#include <wtf/dtoa.h>

No need to add these includes to the header. Please don’t.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:84
> +    static unsigned getPrecision(int m_exponent, unsigned m_precision, unsigned digitsBeforeDecimalPoint);
> +
> +    static int shifting(char* num, int shift);

I don’t think these need to be public functions. They could be private to the .cpp file and use “static” to get external linkage. If we decide to make them public to share them with other code, we probably would not put them in the StringBuilder class. In fact, longer term functions that convert numbers to strings don’t necessarily belong in StringBuilder.

The names here are not normal WebKit function names. We do not use “get” for name of something that returns a value (see WebKit coding style guide), and the name seems too terse. It says “precision”, but precision of what? Needs a name that actually says what it does. And “shifting” is also a curious name for a function. Normally functions that return values are named what they return. If this returns a “shifting” then we have an unconventional use of English grammar here. Also, the argument’s name should not be “num”.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:88
> +    void append(double);
> +
> +    void append(int);

I’m not sure that overloading based on type is quite right here, since the format that is used is based on the type and the dependency on type might be subtle. We may want to give more specific names to these functions as we do to the conversion functions in String.h.

We should refactor this so it shares code with functions in String.h and StringImpl.h.

[Oliver Varga]

Created attachment 113473 [details]
Correcting

I made the previous requests:
 * I corrected the indentation, and rename some variables and functions.
The next step, I will do shortly:
 * I am going to create a new cpp and header file what name is NumericFormatting.h/cpp.
 * It will includes low level numeric formatting functions, that allows higher level classes like String and StringBuilder to use them
   efficiently, so the StringBuilder::append(double) would call the NumericFormatting::doubleToAsciiUsedByDtoa.

[Darin Adler]

(In reply to comment #33)
> NumericFormatting::doubleToAsciiUsedByDtoa.

I like the idea behind this patch but not this name.

For one thing, we spell it ASCII, not Ascii. But also "double to ASCII used by dtoa" is a confusing phrase and I think we can come up with a more straightforward name.

[Darin Adler]

Comment on attachment 113473 [details]
Correcting

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

I appreciate your continuing to work hard on this patch; it’s worth doing. I think there is still some work needed.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:197
> +int StringBuilder::firstDigitOfOriginalNumber(char* numberToShifting, int measurementOfShift)

I don’t understand the meaning of the name “number to shifting".

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:202
> +        numberToShifting[ p ] = numberToShifting[p+1];

Should omit the spaces around the "p". Should include spaces around the "+".

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:218
> +        numberToShifting[i]=0;

Missing spaces here.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:141
> +    static unsigned precisionOfResult(int, unsigned, unsigned);
> +    static int firstDigitOfOriginalNumber(char*, int);

These should go into NumericFormatting.h/cpp, not this class.

I am puzzled. Why is it we need this new code and can’t use the DecimalNumber class that CSSPrimitiveValue’s formatNumber was using?

The argument names here need to be listed. It’s not at all clear what the arguments are.

The name “first digit of original number” does not make it clear that the function will have a side effect of changing the passed in character buffer.

> Source/WebCore/css/CSSPrimitiveValue.cpp:828
> +            text.append(result.toString());

We should make it so you can append one StringBuilder to another. This toString() seems like bad idea.

> Source/WebCore/css/CSSPrimitiveValue.cpp:851
> +            text.append(result.toString());

We should make it so you can append one StringBuilder to another. This toString() seems like bad idea.

> Source/WebCore/css/CSSPrimitiveValue.cpp:871
> +           text.append(result.toString());

We should make it so you can append one StringBuilder to another. This toString() seems like bad idea. Also, incorrect indent here.

> Source/WebCore/css/CSSPrimitiveValue.cpp:961
> +    String resultOfText = text.toString();

I think this could just be named “result”. The phrase “result of text” is not good grammar here. If you really wanted both words it would just be “result text” so, resultText.

[Oliver Varga]

Thank you for your comment, I am goint to check your new instructions shortly.

[Szilard Ledan]

Created attachment 121666 [details]
Correcting v2

Added a new class: NumericFormatting.h/cpp. The StringBuilder::append(double) uses it.

Modified CSSPrimitiveValue::customCssText() to use StringBuilder instead of string.

[WebKit Review Bot]

Comment on attachment 121666 [details]
Correcting v2

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

[Gyuyoung Kim]

Comment on attachment 121666 [details]
Correcting v2

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

[Gustavo Noronha (kov)]

Comment on attachment 121666 [details]
Correcting v2

Attachment 121666 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11187518

[Darin Adler]

Comment on attachment 121666 [details]
Correcting v2

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

I didn’t have time to review the whole thing, but I have a few initial comments.

> Source/JavaScriptCore/wtf/text/NumericFormatting.h:4
> + * Copyright (C) 2011 University of Szeged
> + * Copyright (C) 2011 Oliver Varga
> + * Copyright (C) 2012 Szilard Ledan

Seems like too much copyright for these two function declarations. Did all of these contribute to this?

> Source/JavaScriptCore/wtf/text/NumericFormatting.h:31
> +#include "string.h"

This include is not needed and should be removed, but if it was needed it would be <string.h>.

> Source/JavaScriptCore/wtf/text/NumericFormatting.h:44
> +class NumericFormatting {
> +public:
> +    // Is is used by append(double). Return the precision of the the result number.
> +    unsigned precisionOfResult(int exponent, unsigned precision, unsigned digitsBeforeDecimalPoint);
> +
> +    // Is is used by append(double). If the input number is like '999.999', and the value of the rounding is 0 we have to move forward the carry
> +    // over the decimal point. This kind of rounding implemented the number less then 1 and more than 0, and before the rounding we convert '999.999'
> +    // to '0.999999'. The firstDigitOfOriginalNumber() helps get the real number back.
> +    int firstDigitOfOriginalNumber(char* numberToShifting, int measurementOfShift);
> +};

These are functions, not a class. They could be free functions, or if you really want this to be a class, they should be static member functions, not member functions.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:34
>  #include "config.h"
>  #include "StringBuilder.h"
> +#include "NumericFormatting.h"
> +#include <wtf/MathExtras.h>
> +#include <wtf/dtoa.h>
>  
>  #include "WTFString.h"
>  
> +#include <stdio.h>

This is incorrect. All the includes except for "StringBuilder.h" should be in a single paragraph, and sorted.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:303
> +        StringBuilder::append("-", 1);

This should be:

    append('-');

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:317
> +    printf("debug: exponent:%d precision:%d\n", exponent, precision);

Please don't submit the patch with the printf still here.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:323
> +    int sizeOfsignificand = strlen(significand);

The word significand should be capitalized.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:326
> +    int sizeOfMSignificand = sizeof(significand);

What is this? What does the “M” mean?

[Antonio Gomes]

Comment on attachment 121666 [details]
Correcting v2

based on Darin's review and lack of update.

[Brent Fulgham]

I think Ben Poulain made similar improvements when porting to Arm in 2014. Current sources seem to use StringBuilder.