Closed Bug 1260509 Opened 8 years ago Closed 8 years ago

Implement String.prototype.padStart / padEnd

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mrrrgn, Assigned: mrrrgn)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

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
Assignee: nobody → winter2718
Component: JavaScript Engine → JavaScript: Standard Library
Attached patch padleftright.diff (obsolete) — Splinter Review
Gonna have dinner and flag this for review after I verify that I didn't do something dumb. ^.^
Attached patch padleftright.diff (obsolete) — Splinter Review
Attachment #8736093 - Attachment is obsolete: true
Attachment #8736237 - Flags: review?(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.
Attachment #8736237 - Flags: review?(jorendorff) → review+
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)
(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 !
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
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
Attachment #8736237 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d3ba5b5019c6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.