Bug 72793 – Add flags/precision arguments to String::number(double) to allow fine-grained control over the result string

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Add flags/precision arguments to String::number(double) to allow fine-grained control over the result string.
Use this to replace String::format("%.2f") usage in platform/text/TextStream.cpp, and String::format("%.6lg") in svg/SVGPathStringBuilder.cpp in follow-up patches.
The code is well tested and based upon wtf/dtoa/double-conversion.h. JSC implements toPrecision/toFixed using this code, so I refactored it to make it usable for outside of JSC as well.

"static String number(double, unsigned = ShouldRoundSignificantFigures | ShouldTruncateTrailingZeros, unsigned precision = 6);".

String::format("%.2f", f) -> String::number(f, ShouldRoundDecimalPlaces, 2)
String::format("%.6lg", f) -> String::number(f)

The current implementation of String::number(double) just calls String::format("%.6lg"). This explains the default values for flags & precision, it matches exactly what we're returning right now, but properly rounded using dtoas rounding facilities.

[Nikolas Zimmermann]

Created attachment 115939 [details]
Patch

This patch is tested on Gtk/Mac/Win, during the develoment of a fix for bug 47467.
In order to keep patch sizes small, this first introduces a new variant of String::number, which is then later deployed to fix actual bugs.

[WebKit Review Bot]

Comment on attachment 115939 [details]
Patch

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

New failing tests:
svg/dom/length-list-parser.html

[Nikolas Zimmermann]

CC'ing some reviewers. Can any of you check this patch?

[Zoltan Herczeg]

Comment on attachment 115939 [details]
Patch

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

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:399
> -    DoubleConversionStringBuilder builder(buffer, WTF::NumberToStringBufferLength);
> -    const DoubleToStringConverter& converter = DoubleToStringConverter::EcmaScriptConverter();
> -    builder.Reset();
> -    converter.ToFixed(x, decimalPlaces, &builder);
> -    return JSValue::encode(jsString(exec, UString(builder.Finalize())));
> +    return JSValue::encode(jsString(exec, UString(numberToFixedWidthString(x, decimalPlaces, buffer))));

I think this code now is unsafe. The previous code destroyed "builder" after the return statement, the new code destroys it inside the utility function so accessing the buffer later is unsafe.

[Nikolas Zimmermann]

(In reply to comment #4) 
> I think this code now is unsafe. The previous code destroyed "builder" after the return statement, the new code destroys it inside the utility function so accessing the buffer later is unsafe.

It's not. The double_conversion::StringBuilder that's used in wtf/dtoa/dtoa.cpp (defined in wtf/dtoa/utils.h) does not own the underlying buffer behind the Vector<char> buffer_ it stores.
    class StringBuilder {
    public:
        StringBuilder(char* buffer, int size)
        : buffer_(buffer, size), position_(0) { }

It's passed in here, and thus StringBuilder destruction does not delete the buffer_ in the Vector, nor does calling Finalize() and then destructing the StringBuilder lead to a pointer pointing to invalid memory.

The NumberToStringBuffer is not held by numberToFixedWidth/PrecisionString, but passed in from the outside. As long as that buffer is alive, the pointer returned by Finalize is valid.

[Zoltan Herczeg]

Tricky, but you pass char buffer[WTF::NumberToStringBufferLength]; as an argument not a NumberToStringBuffer.

[Nikolas Zimmermann]

(In reply to comment #6)
> Tricky, but you pass char buffer[WTF::NumberToStringBufferLength]; as an argument not a NumberToStringBuffer.
Yes, as discussed on IRC, this is a leftover, and is equivalent to the NumberToStringBuffer typedef. I'll fix it.

[Nikolas Zimmermann]

As Zoltan asked for it here's a summary of all users of all variants of String::number(int/float/...):

There are 220 users of WebCore.

$ grep -RI String::number * | grep -v ChangeLog | grep -v \.svn | wc -l
   220

in 99 files:

$ String::number * | grep -v ChangeLog | grep -v \.svn | sed -e "s/:.*//" | uniq | wc -l
   99

Here's the full list of all String::number(double) users:

accessibility/:
void AccessibilityRenderObject::changeValueByStep(bool increase)
void AccessibilityRenderObject::changeValueByPercent(float percentChange)

accessiblity/gtk/:
static gboolean webkitAccessibleValueSetCurrentValue(AtkValue* value, const GValue* gValue)

css/:
String CSSAspectRatioValue::customCssText() const
String CSSFlexValue::customCssText() const
String CSSLinearGradientValue::customCssText() const
String CSSRadialGradientValue::customCssText() const
WebKitCSSKeyframeRule* CSSParser::createKeyframeRule(CSSParserValueList* keys)
String CSSPrimitiveValue::customCssText() const
String CSSCubicBezierTimingFunctionValue::customCssText() const

editing/:
void ApplyStyleCommand::applyRelativeFontStyleChange(EditingStyle* style)

html/
void HTMLMeterElement::setMin(double min, ExceptionCode& ec)
void HTMLMeterElement::setMax(double max, ExceptionCode& ec)
void HTMLMeterElement::setValue(double value, ExceptionCode& ec)
void HTMLMeterElement::setLow(double low, ExceptionCode& ec)
void HTMLMeterElement::setHigh(double high, ExceptionCode& ec)
void HTMLMeterElement::setOptimum(double optimum, ExceptionCode& ec)
void HTMLProgressElement::setValue(double value, ExceptionCode& ec)
void HTMLProgressElement::setMax(double max, ExceptionCode& ec)

html/shadow/:
void MediaControlTimelineElement::setPosition(float currentTime)
void MediaControlTimelineElement::setDuration(float duration)
PassRefPtr<MediaControlVolumeSliderElement> MediaControlVolumeSliderElement::create(HTMLMediaElement* mediaElement)
void MediaControlVolumeSliderElement::setVolume(float volume)
PassRefPtr<MediaControlFullscreenVolumeSliderElement> MediaControlFullscreenVolumeSliderElement::create(HTMLMediaElement* mediaElement)

page/:
String PrintContext::pageProperty(Frame* frame, const char* propertyName, int pageNumber)

rendering/:
void RenderLayer::resize(const PlatformMouseEvent& evt, const LayoutSize& oldOffset)

svg/properties/:
SVGPropertyTraits<float>::toString(float)

svg/:
String SVGAngle::valueAsString() const
String SVGAnimatedType::valueAsString()
String SVGLength::valueAsString() const
String SVGNumberList::valueAsString() const
String SVGPointList::valueAsString() const
SVGPropertyTraits<FloatRect>::toString(const FloatRect&)
String SVGTransform::valueAsString() const

xml/:
String Value::toString() const

It turns out its mostly used in functions that are often called by bindings, eg. SVGAngle.valueAsString/etc.. same for all css/ cssText() methods. All calls in html/ are only used via JS.

This doesn't touch any performance sensitive code, as none of them uses String::number.

[Nikolas Zimmermann]

I've did some performance testing on my 64bit MBP, using a JSC release build. I added a doPerfTest() function to wtf/text/WTFString, that compares the old vs. the new style of converting doubles to strings, and made jsc.cpp call it and then immediately exit.

Here's how the testing code looks like:
+    // 123.456
+    {
+        float testNumber = 123.456;
+
+        double startTime = currentTimeMS();
+        for (int i = 0; i < s_iterations; ++i)
+            String::format("%.6lg", testNumber);
+        double runTime = currentTimeMS() - startTime;
+        fprintf(stderr, "Converting 123.456 using old approach took %lfms. avg %lfms/call.\n", runTime, runTime / s_iterations);
+        usleep(s_sleepDelay);
+
+        startTime = currentTimeMS();
+        for (int i = 0; i < s_iterations; ++i)
+            String::number(testNumber);
+        runTime = currentTimeMS() - startTime;
+        fprintf(stderr, "Converting 123.456 using new approach took %lfms. avg %lfms/call.\n", runTime, runTime / s_iterations);
+    }

I've tested it for various numbers, here are the results:
Starting perf test... (iterations: 100000)
Converting 123.456 using old approach took 95.527100ms. avg 0.000955ms/call.
Converting 123.456 using new approach took 28.126953ms. avg 0.000281ms/call.

Converting 123 using old approach took 85.411133ms. avg 0.000854ms/call.
Converting 123 using new approach took 24.190186ms. avg 0.000242ms/call.

Converting 0.1 using old approach took 92.622803ms. avg 0.000926ms/call.
Converting 0.1 using new approach took 23.317871ms. avg 0.000233ms/call.

Converting 1/i using old approach took 106.893066ms. avg 0.001069ms/call.
Converting 1/i using new approach took 27.164062ms. avg 0.000272ms/call.

The new approach is significantly faster in general than the previous snprintf usage, at least on Mac.

[Nikolas Zimmermann]

Created attachment 116207 [details]
Patch v2

Full patch, fixes Zoltans issues and updates one test results.

[Nikolas Zimmermann]

Created attachment 116208 [details]
Patch v3

Oops uploaded wrong version, fixed.

[WebKit Review Bot]

Comment on attachment 116208 [details]
Patch v3

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

New failing tests:
svg/dom/length-list-parser.html

[Zoltan Herczeg]

Comment on attachment 116208 [details]
Patch v3

r=me. Nice speedup.

[Nikolas Zimmermann]

Committed r101056: <http://trac.webkit.org/changeset/101056>

[Nikolas Zimmermann]

Removed win specific baseline in r101063, results can be shared between mac/svg for svg/dom/length-list-parser.html. I suspect the same holds true for Gtk/Qt/Chromium, waiting for their bots to cycle.