Bug 155795 – [JSC] implement String.prototype.padStart() and String.prototype.padEnd() proposal

• Component: New Bugs
• Status: RESOLVED FIXED

[Caitlin Potter (:caitp)]

[JSC] implement String.prototype.padStart() and String.prototype.padEnd() proposal

[Caitlin Potter (:caitp)]

Created attachment 274751 [details]
Patch

[Caitlin Potter (:caitp)]

After the "#padleft" fiasco on twitter this morning, I thought the demand for this in the stdlib was substantial enough that it was worth prototyping in JSC.

I would opt to define most of the work for these methods in a common helper, since they share so much --- so that's probably the next step.

[Build Bot]

Comment on attachment 274751 [details]
Patch

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

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 274754 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 274751 [details]
Patch

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

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 274757 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Caitlin Potter (:caitp)]

Created attachment 274759 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 274775 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 274829 [details]
Patch

[Darin Adler]

Comment on attachment 274829 [details]
Patch

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

> Source/JavaScriptCore/runtime/StringPrototype.cpp:829
> +enum StringPaddingMode {

In newer code we often use an enum class.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:831
> +    kPadStart,
> +    kPadEnd

The style guide for WebKit suggests we not use a “k” prefix for this type of thing.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:832
> +};

Also, why call it “mode”? Why not just call it the padding location, since it’s the location of the padding?

    enum class StringPaddingLocation { Start, End };

> Source/JavaScriptCore/runtime/StringPrototype.cpp:834
> +template <StringPaddingMode mode>

I think we can afford the one to three branches it would cost to have this be a function with an argument rather than a template. I’m not sure it’s worth having an entire separate copy of this function just to avoid a small number of branches in a function that also does some memory allocation, much more expensive than branches. Please consider using a function rather than a function template.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:835
> +static EncodedJSValue stringPadHelper(ExecState* exec)

I suggest naming this padString or padStartOrEnd, rather than "stringPadHelper". We normally use verbs for function names.

In new code, I suggest considering use of "ExecState& state" rather than "ExecState* exec" since it can never be null.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:842
> +    JSString* thisString = thisValue.toString(exec);
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());

Non-obvious, but the more efficient idiom for this in new code is:

    JSString* thisString = thisValue.toStringOrNull(exec);
    if (!thisString)
        return JSValue::encode(jsUndefined());

> Source/JavaScriptCore/runtime/StringPrototype.cpp:847
> +    RELEASE_ASSERT(maxLengthAsDouble >= 0.0 && maxLengthAsDouble == std::trunc(maxLengthAsDouble));

It’s wasteful to do a RELEASE_ASSERT for something that is both guaranteed to be true, and also would also cause no real trouble if it was false, since there is range checking code below that can deal with it perfectly (as long as we don’t unnecessarily convert to uint64_t first).

If you feel the need to assert, then ASSERT, by all means, but I don’t think this is an important thing to assert since it’s an invariant of the result of the toLength function.

With ASSERT, we almost never use &&, because we’d rather use two separate ASSERT. No reason to be unnecessarily vague about which of the two expressions was false, which is what happens when we use &&.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:848
> +    uint64_t maxLength = static_cast<uint64_t>(maxLengthAsDouble);

I do not think it is valuable to convert to a 64-bit integer to do the range check. Instead I suggest we do the range check with the double, and then convert directly from double to unsigned instead of involving uint64_t at all.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:854
> +    // Ensure that the resulting string's length is in the range of int32_t
> +    if (maxLength > std::numeric_limits<int32_t>::max())

Sure would be nice if this limit and other checks like it were based on a maximum JavaScript string length constant rather than explicitly based on int32_t’s range; it’s annoying to have to code in the implicit knowledge that this is the maximum JavaScript string length.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:860
> +        filler = jsSingleCharacterString(exec, ' ');

Maybe we should use nullptr to mean "no fill string argument" and convert that to a space below in the single character special case, instead of actually calling jsSingleCharacterString. A shame to waste any time looking up the JSString for space when all we are going to do is re-extract the first character from it.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:864
> +        filler = fillString.toString(exec);
> +        if (exec->hadException())
> +            return JSValue::encode(jsUndefined());

Non-obvious, but the more efficient idiom for this in new code is:

    filler = fillString.toStringOrNull(exec);
    if (!filler)
        return JSValue::encode(jsUndefined());

> Source/JavaScriptCore/runtime/StringPrototype.cpp:872
> +    if (filler->length() == 1) {
> +        UChar character = filler->value(exec).at(0);

We can use the unsafeView function instead of the value function here to avoid unnecessary string allocation and reference counting. If we combine that with the use of nullptr to mean a space suggested above, this would be:

    if (!filler || filler->length() == 1) {
        UChar character = filler ? filler->unsafeView(exec)[0] : ' ';

> Source/JavaScriptCore/runtime/StringPrototype.cpp:876
> +        if (!(character & ~0xff))
> +            filler = repeatCharacter(exec, static_cast<LChar>(character), fillLength).toString(exec);
> +        else
> +            filler = repeatCharacter(exec, character, fillLength).toString(exec);

It’s wasteful here to call repeatCharacter to make a JSString and convert to a general JSValue, but then call toString to convert back from a JSValue to a JSString*. Instead the repeatCharacter function should be changed to return a JSString*. I think it can return a nullptr when it throws an out of memory exception; although we’d need to be sure to test that doesn’t break stringProtoFuncRepeat.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:878
> +        VM& vm = exec->vm();

Not sure a local variable is best style for this since we are only using it once below.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:890
> +        unsigned repeatCount = fillLength / filler->length();
> +        unsigned remainingCharacters = fillLength % filler->length();
> +        JSRopeString::RopeBuilder ropeBuilder(vm);
> +        for (unsigned i = 0; i < repeatCount; ++i) {
> +            if (!ropeBuilder.append(filler))
> +                return JSValue::encode(throwOutOfMemoryError(exec));
> +        }
> +        if (remainingCharacters) {
> +            if (!ropeBuilder.append(jsSubstring(exec, filler, 0, remainingCharacters)))
> +                return JSValue::encode(throwOutOfMemoryError(exec));
> +        }
> +        filler = ropeBuilder.release();

I think it would be nice to factor this code into a helper function. Might even call it repeatString and have it match the style of repeatCharacter. However, might be better to have it take a RopeBuilder& argument so you can use the same rope to prepend/append thisString to the same rope builder.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:892
> +    ASSERT(!exec->hadException());

Thus assertion will be wrong if repeatCharacter throws an out-of-memory error. We need to include an early exit for that case.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:896
> +    if (mode == kPadEnd)
> +        return JSValue::encode(JSRopeString::create(exec->vm(), thisString, filler));
> +    return JSValue::encode(JSRopeString::create(exec->vm(), filler, thisString));

I think it would be nicer for the non-single-character case to use only a single rope rather than building a first rope and then using that rope to build a second second rope. We’d just append at the start for kPadEnd and at the end for kPadStart. Wasteful to do it in the two separate steps like this.

For the single character case we might still want to keep this code since we won’t already have built a rope.

[Saam Barati]

Comment on attachment 274829 [details]
Patch

I this this would be a lot simpler/easier to write as a builtin in JS

[Darin Adler]

Good point.

[Caitlin Potter (:caitp)]

Created attachment 274903 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 274904 [details]
Patch

[Darin Adler]

Comment on attachment 274904 [details]
Patch

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

> Source/JavaScriptCore/runtime/StringPrototype.cpp:801
> +        if (!ropeBuilder.append(string))
> +            return throwOutOfMemoryError(&exec), nullptr;

We should use a more conventional two line body here:

    if (!ropeBuilder.append(string)) {
        throwOutOfMemoryError(&exec);
        return nullptr;
    }

> Source/JavaScriptCore/runtime/StringPrototype.cpp:806
> +        if (!substr || !ropeBuilder.append(substr))
> +            return throwOutOfMemoryError(&exec), nullptr;

Ditto.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:861
> +enum class StringPaddingLocation {
> +    Start,
> +    End
> +};

I think this reads better on a single line.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:903
> +        UChar character = filler ? filler->view(&exec)[0] : ' ';

This should use the unsafeView function for better efficiency rather than the view function.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:909
> +        filler = repeatStringPattern(exec, fillLength, filler, ropeBuilder);

I think you misunderstood my suggestion. I suggested passing the ropeBuilder in and create a function that would append to it, not so a function that will use it and then call "release". The point of the optimization is to not do a second release.

[Caitlin Potter (:caitp)]

Comment on attachment 274904 [details]
Patch

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

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:903
>> +        UChar character = filler ? filler->view(&exec)[0] : ' ';
> 
> This should use the unsafeView function for better efficiency rather than the view function.

`unsafeView()` is private --- would you prefer making it public, moving `padString` to the JSString class, or adding it as a friend?

[Darin Adler]

Comment on attachment 274904 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/StringPrototype.cpp:903
>>> +        UChar character = filler ? filler->view(&exec)[0] : ' ';
>> 
>> This should use the unsafeView function for better efficiency rather than the view function.
> 
> `unsafeView()` is private --- would you prefer making it public, moving `padString` to the JSString class, or adding it as a friend?

OK, my mistake. OK to use view.

[Caitlin Potter (:caitp)]

Created attachment 274910 [details]
Patch

[Darin Adler]

Comment on attachment 274910 [details]
Patch

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

Those comments about false and nullptr are wrong. I can't delete them due to a bug in how the website works on iPhone.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:802
> +            return false;

nullptr, not false

> Source/JavaScriptCore/runtime/StringPrototype.cpp:809
> +            return false;

nullptr, false

[WebKit Commit Bot]

Comment on attachment 274910 [details]
Patch

Clearing flags on attachment: 274910

Committed r198674: <http://trac.webkit.org/changeset/198674>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

It appears that the tests added with this change are failing:

es6.yaml/es6/String.prototype_methods_String.prototype.padEnd.js.default
es6.yaml/es6/String.prototype_methods_String.prototype.padStart.js.defaul

es6.yaml/es6/String.prototype_methods_String.prototype.padEnd.js.default: ERROR: Unexpected exit code: 136
es6.yaml/es6/String.prototype_methods_String.prototype.padStart.js.default: ERROR: Unexpected exit code: 136

<https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/4538>
<https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/8562>

[Michael Saboff]

Comment on attachment 274910 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:21
> +        * tests/es6.yaml:
> +        * tests/es6/String.prototype_methods_String.prototype.padEnd.js: Added.
> +        * tests/es6/String.prototype_methods_String.prototype.padStart.js: Added.

These tests do not belong in this directory since these functions are stage 3 for ECMAScript 7.

[Caitlin Potter (:caitp)]

Comment on attachment 274910 [details]
Patch

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

(In reply to comment #22)
> It appears that the tests added with this change are failing:
> 
> es6.yaml/es6/String.prototype_methods_String.prototype.padEnd.js.default
> es6.yaml/es6/String.prototype_methods_String.prototype.padStart.js.defaul
> 
> es6.yaml/es6/String.prototype_methods_String.prototype.padEnd.js.default:
> ERROR: Unexpected exit code: 136
> es6.yaml/es6/String.prototype_methods_String.prototype.padStart.js.default:
> ERROR: Unexpected exit code: 136
> 
> <https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/4538>
> <https://build.webkit.org/builders/Apple%20Yosemite%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/8562>

Looking...

>> Source/JavaScriptCore/ChangeLog:21
>> +        * tests/es6/String.prototype_methods_String.prototype.padStart.js: Added.
> 
> These tests do not belong in this directory since these functions are stage 3 for ECMAScript 7.

Fair enough, but there isn't really a more appropriate place to put them yet --- there's a TODO near similar "for future ES spec revisions" entries in es6.yaml, I can add a TODO?

[Caitlin Potter (:caitp)]

I have a fix for the divide-by-zero --- I'm not sure why this didn't occur when running the tests manually ._.

This can be rolled out if it's a problem

[Filip Pizlo]

(In reply to comment #25)
> I have a fix for the divide-by-zero --- I'm not sure why this didn't occur
> when running the tests manually ._.
> 
> This can be rolled out if it's a problem

How soon can the fix land?  Right now the tests are still failing AFAIK.

[Caitlin Potter (:caitp)]

Fix in bug 155903

[Saam Barati]

I still think it'd be worth investigating writing these in JS instead of C++.

[Darin Adler]

I think you are right.

[Yusuke Suzuki]

(In reply to comment #29)
> I think you are right.

I've tried implementing String.prototype.repeat in builtin JS since String.prototype.repeat has similar steps to padStart and padEnd.
And it seems that the current builtin's functionality is good enough to represent these methods.

I'm +1 to implement padStart / padEnd in builtin JS :)

https://bugs.webkit.org/show_bug.cgi?id=155974