[JSC] implement String.prototype.padStart() and String.prototype.padEnd() proposal
Created attachment 274751 [details] Patch
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.
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
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
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
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
Created attachment 274759 [details] Patch
Created attachment 274775 [details] Patch
Created attachment 274829 [details] Patch
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.
Comment on attachment 274829 [details] Patch I this this would be a lot simpler/easier to write as a builtin in JS
Good point.
Created attachment 274903 [details] Patch
Created attachment 274904 [details] Patch
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.
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?
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.
Created attachment 274910 [details] Patch
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
Comment on attachment 274910 [details] Patch Clearing flags on attachment: 274910 Committed r198674: <http://trac.webkit.org/changeset/198674>
All reviewed patches have been landed. Closing bug.
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>
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.
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?
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
(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.
Fix in bug 155903
I still think it'd be worth investigating writing these in JS instead of C++.
I think you are right.
(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