1260509 - Implement String.prototype.padStart / padEnd

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Morgan Phillips [:mrrrgn]]

This string method pads a string with a repeating sequence until the length of the string reaches some max value. The repeated 'filler' sequence may be truncated.

Ex:

> "blahblah".padStart(10, "foo")
'ooblahblah'
> "blahblah".padStart(11, "foo")
'fooblahblah'
> "blahblah".padEnd(10, "foo")
'blahblahfo'
> "blahblah".padEnd(11, "foo")
'blahblahfoo'

See: https://github.com/tc39/proposal-string-pad-start-end

Comment 1

[Morgan Phillips [:mrrrgn]]

Attachment: padleftright.diff

Gonna have dinner and flag this for review after I verify that I didn't do something dumb. ^.^

Comment 2

[Morgan Phillips [:mrrrgn]]

Attachment: padleftright.diff

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8736237 [details] [diff] [review]
padleftright.diff

Review of attachment 8736237 [details] [diff] [review]:
-----------------------------------------------------------------

How's the speed vs. V8? It seems to me like a C++ implementation should be noticeably faster, even for simple stuff like "hi".padStart(8), because every + and += here is (I think) allocating a JSString header. String_repeat uses a trick to reduce the number of concatenations (see steps 8-9).

Also check that String.prototype.padStart.length is 1. I claim it should be 1 because of this in section 17:

> Every built-in Function object, including constructors, has a `length` property
> whose value is an integer. Unless otherwise specified, this value is equal to the
> largest number of named arguments shown in the subclause headings for the function
> description. Optional parameters (which are indicated with brackets: [ ]) or rest
> parameters (which are shown using the form «...name») are not included in the
> default argument count.

Also see NOTE 2 immediately under that.

::: js/src/builtin/String.js
@@ +29,5 @@
> +    if (filler === "")
> +        return str;
> +
> +    // Step 9.
> +    let fillLen = maxLength - strLen;

intMaxLength, not maxLength. I guess we would need a test with fractional maxLength, or one that tests how many times .toString()/.valueOf() gets called, to catch this?

::: js/src/tests/ecma_7/String/string-pad-start-end.js
@@ +25,5 @@
> +
> +assertEq("42bloop", String.prototype.padEnd.call(proxy, 7, "bloopie"));
> +
> +if (typeof reportCompare === "function")
> +    reportCompare(true, true);

Move this to the end, please.

Comment 4

[ljharb]

The functions' length absolutely is `1`. Please also note yesterday's spec changes: passing a fillStr that toStrings to the empty string causes the string to be returned unchanged. I'm working on the test262 tests right now (https://github.com/tc39/test262/pull/564)

Comment 5

[Morgan Phillips [:mrrrgn]]

(In reply to ljharb from comment #4)
> The functions' length absolutely is `1`. Please also note yesterday's spec
> changes: passing a fillStr that toStrings to the empty string causes the
> string to be returned unchanged. I'm working on the test262 tests right now
> (https://github.com/tc39/test262/pull/564)

Great, I'll add that before landing. Thanks !

Comment 6

[Morgan Phillips [:mrrrgn]]

Test added. We're compliant.

As for performance, v8 is also self hosted and our implementations are very similar (only so many ways to skin a cat).

I'm building v8 currently, here's what I get for our implementation:

for (let i = 1; i < 11; i++) {
    let t0 = Date.now();
    mrrrgn .padStart(STEP_SIZE * i, foo);
    let t1 = Date.now();
    console.log();
}

Morgans-MacBook-Pro:_DBG.OBJ mrrrgn$ dist/bin/js ../../../../test-scripts/padspeed.js
10000 took 0.007 S
20000 took 0.004 S
30000 took 0.005 S
40000 took 0.007 S
50000 took 0.008 S
60000 took 0.008 S
70000 took 0.005 S
80000 took 0.009 S
90000 took 0.007 S
100000 took 0.009 S
Morgans-MacBook-Pro:_DBG.OBJ mrrrgn$
Morgans-MacBook-Pro:_DBG.OBJ mrrrgn$ dist/bin/js ../../../../test-scripts/padspeed.js
100000 took 0.02 S
200000 took 0.022 S
300000 took 0.024 S
400000 took 0.031 S
500000 took 0.039 S
600000 took 0.041 S
700000 took 0.054 S
800000 took 0.06 S
900000 took 0.066 S
1000000 took 0.207 S

Comment 7

[Morgan Phillips [:mrrrgn]]

Replacing my "repeat" for loop with a call to String_repeat improved the time. I tested v8 and we're running with the same performance.

Morgans-MacBook-Pro:_DBG.OBJ mrrrgn$ dist/bin/js ../../../../test-scripts/padspeed.js
padStart: 100000 took 0.004 S
padStart: 200000 took 0 S
padStart: 300000 took 0.001 S
padStart: 400000 took 0 S
padStart: 500000 took 0.001 S
padStart: 600000 took 0 S
padStart: 700000 took 0 S
padStart: 800000 took 0.001 S
padStart: 900000 took 0 S
padStart: 1000000 took 0 S
padStart: 1100000 took 0.002 S
padStart: 1200000 took 0 S
padStart: 1300000 took 0 S
padStart: 1400000 took 0 S
padStart: 1500000 took 0 S
padStart: 1600000 took 0 S
padStart: 1700000 took 0 S
padStart: 1800000 took 0 S
padStart: 1900000 took 0 S
padStart: 2000000 took 0 S

Comment 8

[Morgan Phillips [:mrrrgn]]

Attachment: padleftright.diff

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ba5b5019c6

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d3ba5b5019c6

Comment 11

[Julien G]

Documentation written down here:
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/padStart
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/padEnd