Bug 1406311 - sprintfjs: optimise string-format for %S patterns;r=bgrins
authorJulian Descottes <jdescottes@mozilla.com>
Tue, 17 Oct 2017 21:11:48 +0200
changeset 386820 41ab5d21e50a0cd2458e4e171c5e2026f2e96bc3
parent 386819 66d602064c8b59aec1d2f4337dac6003e8445ff3
child 386821 3915770a8869399ecde1c32d7c64adb61112524a
push id96311
push userarchaeopteryx@coole-files.de
push dateWed, 18 Oct 2017 09:52:02 +0000
treeherdermozilla-inbound@a8a1e8cc1980 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbgrins
bugs1406311
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1406311 - sprintfjs: optimise string-format for %S patterns;r=bgrins MozReview-Commit-ID: AOi3cUTedX9
devtools/shared/sprintfjs/UPGRADING.md
devtools/shared/sprintfjs/sprintf.js
--- a/devtools/shared/sprintfjs/UPGRADING.md
+++ b/devtools/shared/sprintfjs/UPGRADING.md
@@ -1,12 +1,17 @@
 SPRINTF JS UPGRADING
 
 Original library at https://github.com/alexei/sprintf.js
+
+This library should no longer be upgraded from upstream. We added performance improvements
+in https://bugzilla.mozilla.org/show_bug.cgi?id=1406311. Most importantly removing the
+usage of the get_type() method as well as prioritizing the %S use case.
+
+If for some reason, updating from upstream becomes necessary, please refer to the bug
+mentioned above to reimplement the performance fixes in the new version.
+
 By default the library only supports string placeholders using %s (lowercase) while we use
-%S (uppercase). The library has to be manually patched in order to support it.
+%S (uppercase). The library also has to be manually patched in order to support it.
 
 - grab the unminified version at https://github.com/alexei/sprintf.js/blob/master/src/sprintf.js
 - update the re.placeholder regexp to allow "S" as well as "s"
 - update the switch statement in the format() method to make case "S" equivalent to case "s"
-
-The original changeset adding support for "%S" can be found on this fork:
-- https://github.com/juliandescottes/sprintf.js/commit/a60ea5d7c4cd9a006002ba9f0afc1e2689107eec
\ No newline at end of file
--- a/devtools/shared/sprintfjs/sprintf.js
+++ b/devtools/shared/sprintfjs/sprintf.js
@@ -82,16 +82,31 @@
                 }
                 else if (match[1]) { // positional argument (explicit)
                     arg = argv[match[1]]
                 }
                 else { // positional argument (implicit)
                     arg = argv[cursor++]
                 }
 
+                // The most commonly used placeholder in DevTools is the string (%S or %s).
+                // We check it first to avoid unnecessary verifications.
+                let hasPadding = match[6];
+                let patternType = match[8];
+                if (!hasPadding && (patternType === "S" || patternType === "s")) {
+                    if (typeof arg === "function") {
+                        arg = arg();
+                    }
+                    if (typeof arg !== "string") {
+                        arg = String(arg);
+                    }
+                    output[output.length] = match[7] ? arg.substring(0, match[7]) : arg;
+                    continue;
+                }
+
                 if (re.not_type.test(match[8]) && re.not_primitive.test(match[8]) && typeof arg == 'function') {
                     arg = arg()
                 }
 
                 if (re.numeric_arg.test(match[8]) && (typeof arg != 'number' && isNaN(arg))) {
                     throw new TypeError(sprintf("[sprintf] expecting number but found %s", typeof arg))
                 }