Bug 1406311 - sprintfjs: optimise string-format for %S patterns;r=bgrins draft
authorJulian Descottes <jdescottes@mozilla.com>
Tue, 17 Oct 2017 21:11:48 +0200
changeset 681816 2e469aa4ee4cb2df867e3eb328454aad3dc26a2e
parent 681763 9c249323612995278007b2ecfc9a3ca991dcd81b
child 736249 7ff77f1a81affe2b5f0f6c98c1ac862f48b2fda0
push id84941
push userjdescottes@mozilla.com
push dateTue, 17 Oct 2017 20:57:10 +0000
reviewersbgrins
bugs1406311
milestone58.0a1
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))
                 }