path: replace "magic" numbers by readable constants #18654

daynin · 2018-02-08T20:02:57Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

path

I replaced "magic" numbers from "path" module by well readable constants. I want to replace all "magic" numbers in whole project by constants like these, but I decided to start from only one module "path"

BridgeAR

Hm, the name symbols is actually misleading out of my perspective. For me a symbol would mean it is e.g. Symbol('foo'). I think it should be constants instead.

Besides that I am not sure if V8 inlines these entries properly or not. Would you be so kind and run a benchmark? :-)

richardlau · 2018-02-08T20:26:39Z

lib/internal/symbols.js

+  // non-alphabetic symbols
+  DOT: 46, /*.*/
+  SLASH: 47, /*/*/
+  VERTICAL_BAR: 92, /*|*/


92 should be /*\*/ (backslash).

@richardlau yeah, my bad, it's "backslash" of course

apapirovski

I like the idea here but I agree with @BridgeAR that the file naming is less than ideal. I think the symbols (after renaming) should be destructured into constants, so that they can be used without symbols. which needs to perform a lookup. That also means that the naming of the properties should be more sensible... perhaps "CHAR_UPPERCASE_A" (& "CHAR_LOWERCASE_A"), "CHAR_FORWARD_SLASH", "CHAR_BACKWARD_SLASH", etc.

daynin · 2018-02-09T15:36:22Z

@apapirovski @richardlau @BridgeAR Hi! I renamed module "symbols" into "constants" and constants. Also I ran benchmarks and this is a result:

                                                                                                    confidence improvement accuracy (*)   (**)  (***)
 path/basename-posix.js n=1000000 pathext=""                                                                        0.10 %       ±1.06% ±1.42% ±1.85%
 path/basename-posix.js n=1000000 pathext="/"                                                                       0.06 %       ±0.65% ±0.87% ±1.13%
 path/basename-posix.js n=1000000 pathext="/foo"                                                                   -0.27 %       ±0.50% ±0.66% ±0.87%
 path/basename-posix.js n=1000000 pathext="/foo/.bar.baz"                                                          -0.08 %       ±0.35% ±0.46% ±0.60%
 path/basename-posix.js n=1000000 pathext="/foo/.bar.baz|.baz"                                               *     -0.23 %       ±0.21% ±0.27% ±0.36%
 path/basename-posix.js n=1000000 pathext="/foo/bar/baz/asdf/quux.html"                                             0.03 %       ±0.29% ±0.38% ±0.50%
 path/basename-posix.js n=1000000 pathext="/foo/bar/baz/asdf/quux.html|.html"                                       0.07 %       ±0.18% ±0.23% ±0.30%
 path/basename-posix.js n=1000000 pathext="foo"                                                                    -0.15 %       ±0.42% ±0.56% ±0.72%
 path/basename-posix.js n=1000000 pathext="foo/bar."                                                               -0.33 %       ±0.40% ±0.53% ±0.70%
 path/basename-posix.js n=1000000 pathext="foo/bar.|."                                                             -0.05 %       ±0.25% ±0.34% ±0.44%
 path/basename-win32.js n=1000000 pathext=""                                                                 *     -0.99 %       ±0.94% ±1.25% ±1.63%
 path/basename-win32.js n=1000000 pathext="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html"                             -0.16 %       ±0.17% ±0.22% ±0.29%
 path/basename-win32.js n=1000000 pathext="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html|.html"                        0.23 %       ±0.27% ±0.37% ±0.48%
 path/basename-win32.js n=1000000 pathext="C:\\\\"                                                                 -0.62 %       ±0.63% ±0.83% ±1.09%
 path/basename-win32.js n=1000000 pathext="C:\\\\foo"                                                              -0.29 %       ±0.35% ±0.47% ±0.61%
 path/basename-win32.js n=1000000 pathext="D:\\\\foo\\\\.bar.baz"                                                  -0.23 %       ±0.36% ±0.48% ±0.63%
 path/basename-win32.js n=1000000 pathext="E:\\\\foo\\\\.bar.baz|.baz"                                              0.07 %       ±0.19% ±0.25% ±0.33%
 path/basename-win32.js n=1000000 pathext="foo"                                                                    -0.18 %       ±0.36% ±0.48% ±0.63%
 path/basename-win32.js n=1000000 pathext="foo\\\\bar."                                                    ***     -0.58 %       ±0.32% ±0.42% ±0.55%
 path/basename-win32.js n=1000000 pathext="foo\\\\bar.|."                                                   **     -0.39 %       ±0.27% ±0.36% ±0.47%
 path/dirname-posix.js n=1000000 path=""                                                                           -1.11 %       ±1.46% ±1.95% ±2.54%
 path/dirname-posix.js n=1000000 path="/"                                                                   **     -1.38 %       ±0.84% ±1.12% ±1.46%
 path/dirname-posix.js n=1000000 path="/foo"                                                                       -0.34 %       ±0.65% ±0.87% ±1.13%
 path/dirname-posix.js n=1000000 path="/foo/bar"                                                                    0.58 %       ±0.64% ±0.85% ±1.11%
 path/dirname-posix.js n=1000000 path="/foo/bar/baz/asdf/quux"                                                      0.08 %       ±0.33% ±0.44% ±0.57%
 path/dirname-posix.js n=1000000 path="foo"                                                                         0.17 %       ±1.05% ±1.40% ±1.82%
 path/dirname-posix.js n=1000000 path="foo/bar"                                                                    -0.05 %       ±0.66% ±0.88% ±1.15%
 path/dirname-win32.js n=1000000 path=""                                                                            0.11 %       ±0.94% ±1.25% ±1.63%
 path/dirname-win32.js n=1000000 path="\\\\"                                                                        0.16 %       ±0.79% ±1.05% ±1.36%
 path/dirname-win32.js n=1000000 path="\\\\foo"                                                                    -0.25 %       ±0.39% ±0.52% ±0.67%
 path/dirname-win32.js n=1000000 path="C:\\\\foo\\\\bar"                                                           -0.15 %       ±0.36% ±0.47% ±0.62%
 path/dirname-win32.js n=1000000 path="D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux"                                    -0.07 %       ±0.26% ±0.35% ±0.45%
 path/dirname-win32.js n=1000000 path="foo"                                                                         0.03 %       ±0.41% ±0.55% ±0.71%
 path/dirname-win32.js n=1000000 path="foo\\\\bar"                                                                 -0.13 %       ±0.41% ±0.55% ±0.71%
 path/extname-posix.js n=1000000 path=""                                                                           -1.10 %       ±1.18% ±1.57% ±2.05%
 path/extname-posix.js n=1000000 path="/"                                                                          -0.24 %       ±0.88% ±1.17% ±1.53%
 path/extname-posix.js n=1000000 path="/foo"                                                                       -0.42 %       ±0.59% ±0.79% ±1.04%
 path/extname-posix.js n=1000000 path="/foo/bar/baz/asdf/quux.foobarbazasdfquux"                                   -0.00 %       ±0.12% ±0.16% ±0.21%
 path/extname-posix.js n=1000000 path="/foo/bar/baz/asdf/quux"                                                      0.08 %       ±0.51% ±0.67% ±0.88%
 path/extname-posix.js n=1000000 path="foo/.bar.baz"                                                                0.21 %       ±0.26% ±0.35% ±0.46%
 path/extname-posix.js n=1000000 path="foo/bar/...baz.quux"                                                        -0.14 %       ±0.18% ±0.25% ±0.32%
 path/extname-posix.js n=1000000 path="foo/bar/..baz.quux"                                                   *     -0.19 %       ±0.17% ±0.22% ±0.29%
 path/extname-posix.js n=1000000 path="index.html"                                                                  0.26 %       ±0.36% ±0.48% ±0.63%
 path/extname-posix.js n=1000000 path="index"                                                                       0.28 %       ±0.49% ±0.66% ±0.87%
 path/extname-win32.js n=1000000 path=""                                                                           -0.29 %       ±1.26% ±1.68% ±2.19%
 path/extname-win32.js n=1000000 path="\\\\"                                                                       -0.28 %       ±0.93% ±1.24% ±1.62%
 path/extname-win32.js n=1000000 path="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.foobarbazasdfquux"                     0.01 %       ±0.11% ±0.15% ±0.20%
 path/extname-win32.js n=1000000 path="C:\\\\foo"                                                           **     -0.59 %       ±0.39% ±0.53% ±0.68%
 path/extname-win32.js n=1000000 path="D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux"                                    -0.14 %       ±0.43% ±0.57% ±0.74%
 path/extname-win32.js n=1000000 path="foo\\\\.bar.baz"                                                            -0.11 %       ±0.32% ±0.43% ±0.56%
 path/extname-win32.js n=1000000 path="foo\\\\bar\\\\...baz.quux"                                          ***     -0.45 %       ±0.24% ±0.32% ±0.42%
 path/extname-win32.js n=1000000 path="foo\\\\bar\\\\..baz.quux"                                                   -0.08 %       ±0.21% ±0.28% ±0.36%
 path/extname-win32.js n=1000000 path="index.html"                                                                  0.07 %       ±0.30% ±0.40% ±0.52%
 path/extname-win32.js n=1000000 path="index"                                                                *     -0.61 %       ±0.46% ±0.61% ±0.80%
 path/format-posix.js n=10000000 props="/|/home/user/dir|index.html|.html|index"                                    0.17 %       ±0.43% ±0.57% ±0.74%
 path/format-win32.js n=10000000 props="C:\\\\|C:\\\\path\\\\dir|index.html|.html|index"                           -0.05 %       ±0.29% ±0.38% ±0.50%
 path/isAbsolute-posix.js n=1000000 path="."                                                                        0.15 %       ±1.55% ±2.07% ±2.69%
 path/isAbsolute-posix.js n=1000000 path=""                                                                         0.95 %       ±1.62% ±2.15% ±2.80%
 path/isAbsolute-posix.js n=1000000 path="/baz/.."                                                                 -1.55 %       ±1.70% ±2.26% ±2.95%
 path/isAbsolute-posix.js n=1000000 path="/foo/bar"                                                                -0.07 %       ±1.62% ±2.15% ±2.80%
 path/isAbsolute-posix.js n=1000000 path="bar/baz"                                                                 -1.10 %       ±1.54% ±2.05% ±2.67%
 path/isAbsolute-win32.js n=1000000 path="."                                                                       -0.55 %       ±1.11% ±1.47% ±1.92%
 path/isAbsolute-win32.js n=1000000 path=""                                                                        -0.21 %       ±1.33% ±1.77% ±2.31%
 path/isAbsolute-win32.js n=1000000 path="//server"                                                                 0.67 %       ±1.56% ±2.08% ±2.70%
 path/isAbsolute-win32.js n=1000000 path="bar\\\\baz"                                                              -0.84 %       ±0.87% ±1.16% ±1.51%
 path/isAbsolute-win32.js n=1000000 path="C:\\\\baz\\\\.."                                                         -0.50 %       ±0.69% ±0.92% ±1.20%
 path/isAbsolute-win32.js n=1000000 path="C:baz\\\\.."                                                             -0.50 %       ±0.74% ±0.98% ±1.28%
 path/join-posix.js n=1000000 paths="/foo|bar||baz/asdf|quux|.."                                                    0.08 %       ±0.15% ±0.20% ±0.26%
 path/join-win32.js n=1000000 paths="C:\\\\foo|bar||baz\\\\asdf|quux|.."                                           -0.32 %       ±0.61% ±0.82% ±1.08%
 path/makeLong-win32.js n=1000000 path="\\\\\\\\?\\\\foo"                                                          -0.11 %       ±0.26% ±0.35% ±0.45%
 path/makeLong-win32.js n=1000000 path="\\\\\\\\foo\\\\bar"                                                         0.03 %       ±0.48% ±0.64% ±0.84%
 path/makeLong-win32.js n=1000000 path="C:\\\\foo"                                                                  0.05 %       ±0.28% ±0.37% ±0.48%
 path/makeLong-win32.js n=1000000 path="foo\\\\bar"                                                                 0.18 %       ±0.55% ±0.74% ±0.96%
 path/normalize-posix.js n=1000000 path="."                                                                        -0.33 %       ±0.34% ±0.45% ±0.59%
 path/normalize-posix.js n=1000000 path=""                                                                         -0.39 %       ±1.41% ±1.88% ±2.45%
 path/normalize-posix.js n=1000000 path="/../"                                                                     -0.16 %       ±0.24% ±0.32% ±0.41%
 path/normalize-posix.js n=1000000 path="/foo"                                                                     -0.01 %       ±0.41% ±0.55% ±0.72%
 path/normalize-posix.js n=1000000 path="/foo/bar"                                                          **     -0.39 %       ±0.25% ±0.33% ±0.43%
 path/normalize-posix.js n=1000000 path="/foo/bar//baz/asdf/quux/.."                                         *      0.10 %       ±0.10% ±0.13% ±0.17%
 path/normalize-win32.js n=1000000 path="."                                                                         0.10 %       ±0.46% ±0.62% ±0.81%
 path/normalize-win32.js n=1000000 path=""                                                                         -0.04 %       ±1.07% ±1.43% ±1.86%
 path/normalize-win32.js n=1000000 path="C:\\\\..\\\\"                                                       *     -0.32 %       ±0.29% ±0.39% ±0.50%
 path/normalize-win32.js n=1000000 path="C:\\\\foo"                                                                -0.06 %       ±0.38% ±0.51% ±0.67%
 path/normalize-win32.js n=1000000 path="C:\\\\foo\\\\bar"                                                         -0.01 %       ±0.34% ±0.45% ±0.59%
 path/normalize-win32.js n=1000000 path="C:\\\\foo\\\\bar\\\\\\\\baz\\\\asdf\\\\quux\\\\.."                        -0.24 %       ±0.33% ±0.44% ±0.58%
 path/parse-posix.js n=1000000 path=""                                                                             -0.38 %       ±1.49% ±1.98% ±2.58%
 path/parse-posix.js n=1000000 path="/"                                                                            -0.71 %       ±3.22% ±4.28% ±5.57%
 path/parse-posix.js n=1000000 path="/foo"                                                                         -0.22 %       ±0.46% ±0.61% ±0.80%
 path/parse-posix.js n=1000000 path="/foo/bar.baz"                                                                  0.33 %       ±0.44% ±0.59% ±0.76%
 path/parse-posix.js n=1000000 path="/foo/bar/baz/asdf/.quux"                                                       0.05 %       ±0.20% ±0.27% ±0.35%
 path/parse-posix.js n=1000000 path="foo/.bar.baz"                                                                  0.05 %       ±0.36% ±0.48% ±0.62%
 path/parse-posix.js n=1000000 path="foo/bar"                                                                       0.05 %       ±0.38% ±0.51% ±0.66%
 path/parse-win32.js n=1000000 path=""                                                                             -0.40 %       ±0.98% ±1.30% ±1.70%
 path/parse-win32.js n=1000000 path="\\\\foo"                                                                *      0.44 %       ±0.44% ±0.58% ±0.76%
 path/parse-win32.js n=1000000 path="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\.quux"                                       -0.15 %       ±0.27% ±0.35% ±0.46%
 path/parse-win32.js n=1000000 path="C:\\\\"                                                                        0.50 %       ±0.93% ±1.24% ±1.62%
 path/parse-win32.js n=1000000 path="C:\\\\foo"                                                                     0.38 %       ±0.54% ±0.72% ±0.94%
 path/parse-win32.js n=1000000 path="E:\\\\foo\\\\bar.baz"                                                          0.04 %       ±0.14% ±0.19% ±0.24%
 path/parse-win32.js n=1000000 path="foo\\\\.bar.baz"                                                               0.09 %       ±0.29% ±0.39% ±0.50%
 path/parse-win32.js n=1000000 path="foo\\\\bar"                                                                   -0.06 %       ±0.38% ±0.51% ±0.66%
 path/relative-posix.js n=1000000 paths="/|/"                                                                       0.30 %       ±1.07% ±1.43% ±1.87%
 path/relative-posix.js n=1000000 paths="/|/var"                                                                   -0.01 %       ±0.32% ±0.43% ±0.56%
 path/relative-posix.js n=1000000 paths="/data/orandea/test/aaa|/data/orandea/impl/bbb"                             0.06 %       ±0.16% ±0.21% ±0.28%
 path/relative-posix.js n=1000000 paths="/foo/bar/baz/quux|/"                                                      -0.13 %       ±0.29% ±0.39% ±0.51%
 path/relative-posix.js n=1000000 paths="/foo/bar/baz/quux|/foo/bar/baz/quux"                                       0.61 %       ±0.91% ±1.21% ±1.58%
 path/relative-posix.js n=1000000 paths="/foo/bar/baz/quux|/var/log"                                                0.03 %       ±0.23% ±0.30% ±0.39%
 path/relative-posix.js n=1000000 paths="/var|/bin"                                                                 0.19 %       ±0.45% ±0.59% ±0.77%
 path/relative-win32.js n=1000000 paths="C:\\\\|D:\\\\"                                                             0.02 %       ±0.15% ±0.20% ±0.25%
 path/relative-win32.js n=1000000 paths="C:\\\\foo\\\\bar\\\\baz\\\\quux|C:\\\\"                                    0.72 %       ±0.80% ±1.08% ±1.43%
 path/relative-win32.js n=1000000 paths="C:\\\\foo\\\\bar\\\\baz|C:\\\\foo\\\\bar\\\\baz"                          -0.29 %       ±0.75% ±1.00% ±1.31%
 path/relative-win32.js n=1000000 paths="C:\\\\foo\\\\BAR\\\\BAZ|C:\\\\foo\\\\bar\\\\baz"                           0.12 %       ±0.27% ±0.36% ±0.47%
 path/relative-win32.js n=1000000 paths="C:\\\\orandea\\\\test\\\\aaa|C:\\\\orandea\\\\impl\\\\bbb"                 0.08 %       ±0.35% ±0.46% ±0.61%
 path/resolve-posix.js n=1000000 paths=""                                                                   **      1.31 %       ±0.91% ±1.22% ±1.60%
 path/resolve-posix.js n=1000000 paths="|"                                                                          0.47 %       ±1.52% ±2.03% ±2.68%
 path/resolve-posix.js n=1000000 paths="a/b/c/|../../.."                                                    **      1.26 %       ±0.79% ±1.06% ±1.40%
 path/resolve-posix.js n=1000000 paths="foo/bar|/tmp/file/|..|a/../subfile"                                        -0.05 %       ±0.09% ±0.13% ±0.17%
 path/resolve-win32.js n=1000000 paths=""                                                                           0.27 %       ±0.91% ±1.22% ±1.58%
 path/resolve-win32.js n=1000000 paths="|"                                                                          0.25 %       ±0.72% ±0.96% ±1.25%
 path/resolve-win32.js n=1000000 paths="c:/blah\\\\blah|d:/games|c:../a"                                           -0.22 %       ±0.44% ±0.60% ±0.79%
 path/resolve-win32.js n=1000000 paths="c:/ignore|d:\\\\a/b\\\\c/d|\\\\e.exe"                                       0.30 %       ±0.49% ±0.66% ±0.87%

Looks like fluctuations. What do you think?

BridgeAR

LGTM. I like the approach. Just two small suggestions.

BridgeAR · 2018-02-09T15:44:29Z

lib/internal/constants.js

+  CHAR_UPPERCASE_Z: 90, /*Z*/
+  CHAR_LOWERCASE_Z: 122, /*z*/
+
+  // non-alphabetic symbols


Non blocking nit: it would be great to uppercase and punctuate comments :-)

BridgeAR · 2018-02-09T15:45:27Z

lib/path.js

-                  if (code === 47/*/*/ || code === 92/*\*/)
+                  const isPathSeparator =
+                    code === CHAR_FORWARD_SLASH || code === CHAR_BACKWARD_SLASH;
+                  if (isPathSeparator)


Non blocking nit: I would just use a if statement spread over two lines.

BridgeAR · 2018-02-09T16:11:59Z

CI https://ci.nodejs.org/job/node-test-pull-request/13064/

apapirovski

LGTM. I think this makes the code a lot clearer.

devsnek

i'm -0 on this change, imo current setup looks fine

devsnek · 2018-02-09T16:50:32Z

lib/internal/constants.js

+
+module.exports = {
+  // Alphabet chars.
+  CHAR_UPPERCASE_A: 65, /*A*/


nit: for readability it might help to change these comments to // style

@devdazed what about comment like this: // /? Now it's /*/*/

this is probably more subjective than i realized, i'll leave it up to your judgement 👍

I would at least put spaces and quotes around them like /* 'A' */ which also makes the original suggestion less crazy since it would be // '/' which is at least a little clearer.

apapirovski · 2018-02-09T16:52:15Z

i'm -0 on this change, imo current setup looks fine

The "changes requested" makes the -0 effectively a -1 and the "nit" non optional... 😕

devsnek · 2018-02-09T16:53:56Z

my intention was more like "i think its fine the way it is but if it does get changed perhaps these comments could be cleaned up", please don't consider the above blocking this pr, sorry for the confusion 😄

apapirovski · 2018-02-09T16:56:12Z

my intention was more like "i think its fine the way it is but if it does get changed perhaps these comments could be cleaned up", sorry for the confusion 😄

That's fine but the "changes requested" means this won't be able to land as is.

FWIW, I considered making the same comment re: the comment style but arguably /*/*/ is clearer than // /.

didn't mean to block this pr

daynin · 2018-02-09T19:18:17Z

@BridgeAR @apapirovski @richardlau Can I fix it somehow?

BridgeAR · 2018-02-09T19:32:50Z

@daynin our tests currently have a couple of flakes and therefore the CI some times fail even though the code change is completely fine.

These failures are independent and you do not have to worry about them :-)

apapirovski · 2018-02-09T19:33:30Z

The Windows failures seem related but I struggle to find where exactly this change breaks them.

kfarnung · 2018-02-09T19:45:14Z

lib/internal/constants.js

@@ -0,0 +1,16 @@
+'use strict';
+
+module.exports = {


Does it make sense to group the constants a little more? This module looks like it's a place where more constants could be added so maybe it makes sense to create to group them in a character or char object?

In general I agree but I personally think we should do that when it is time to do so instead of adding something before we need it.

@kfarnung I agree with @BridgeAR. Besides, I want to refactor similar places in the codebase, so I think I'll do it next time.

If we're planning to add more in the future, it makes the scope of any future change smaller and less risky. I don't have a strong feeling either way, mostly just an observation since the new file is simply internal/constants.js and in my experience those sorts of files get out of control before you know it.

apapirovski · 2018-02-09T19:50:19Z

lib/path.js

-      } else if ((code >= 65/*A*/ && code <= 90/*Z*/) ||
-                 (code >= 97/*a*/ && code <= 122/*z*/)) {
+      } else if ((code >= CHAR_UPPERCASE_A && code <= CHAR_UPPERCASE_Z) ||
+                 (code >= CHAR_LOWERCASE_A && code <= CHAR_UPPERCASE_Z)) {


Typo here. Second line of the else if statement, second variable is supposed to be CHAR_LOWERCASE_Z.

@apapirovski fixed

Oh, I did not see that you already commented @apapirovski :D

BridgeAR · 2018-02-09T19:54:57Z

lib/path.js

-      } else if ((code >= 65/*A*/ && code <= 90/*Z*/) ||
-                 (code >= 97/*a*/ && code <= 122/*z*/)) {
+      } else if ((code >= CHAR_UPPERCASE_A && code <= CHAR_UPPERCASE_Z) ||
+                 (code >= CHAR_LOWERCASE_A && code <= CHAR_UPPERCASE_Z)) {


The second case is faulty.

apapirovski · 2018-02-09T19:55:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13068/

daynin · 2018-02-10T07:52:57Z

@apapirovski @BridgeAR Should i fix it or just wait?

Fishrock123

I like this, my only though would be if we want this constants file to be more namespaced or not.

BridgeAR · 2018-02-12T19:13:12Z

Landed in 6abce37 🎉

@daynin congratulations on your first commit to Node.js!

PR-URL: nodejs#18654 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

gideontee · 2018-02-12T20:29:47Z

@BridgeAR Just to understand the philosophy of the coding style, why do we compare characters using s.charCodeAt(i) instead of just s.charAt(i). Is it because of performance? If it isn't, wouldn't keeping the comparison of characters in its character form more readable.

Looking forward into the future, it would mean that potentially there might be a mapping of all ASCII characters to its equivalent decimal values.

apapirovski · 2018-02-12T20:33:56Z

It's still about 3x as fast to use .charCodeAt() with === comparison than .charAt(). The real question is whether bracket ([i]) access is as fast or faster now.

apapirovski · 2018-02-12T20:42:58Z

And I've gone ahead and answered my question — it's still slower by about 10-20%.

Looking forward into the future, it would mean that potentially there might be a mapping of all ASCII characters to its equivalent decimal values.

That's somewhat unlikely given that we don't have that many places where this is an issue.

gideontee · 2018-02-12T20:51:41Z

https://jsperf.com/charcodeat-vs-string-comparison/11

I'm not an expert in JavaScript performance testing, but it seems here that character equality using bracket [i] access seems the fastest. Is this a reliable benchmark?

apapirovski · 2018-02-12T20:53:44Z

@jiaherrt Not really. It accesses the same character in a loop. Wouldn't surprise me if V8 can inline that access. I tested it on the function modified in this PR using the benchmarks within the benchmark folder which yielded roughly 10-20% worse results, depending on the exact test case.

daynin · 2018-02-12T22:07:16Z

@BridgeAR

@daynin congratulations on your first commit to Node.js!

Thank you!

gideontee · 2018-02-12T22:10:50Z

@daynin, are you planning on refactoring the other constants anytime soon? Would it be okay if I start refactoring the others?

daynin · 2018-02-12T22:49:55Z

@jiaherrt hi!

@daynin, are you planning on refactoring the other constants anytime soon?

Yeah, I wanna fix it in other places too. But if you want you can do the same

gideontee · 2018-02-12T23:00:32Z

Alright. I'll start with lib/url.js and related files. Thank you

apapirovski · 2018-02-12T23:06:52Z

@jiaherrt Just a note to say that if you're working on lib/url.js, there are some pretty ugly bits there that could likely be improved without sacrificing performance. For example,

if (protocol.length >= 4 &&
               protocol.charCodeAt(0) === 102/*f*/ &&
               protocol.charCodeAt(1) === 105/*i*/ &&
               protocol.charCodeAt(2) === 108/*l*/ &&
               protocol.charCodeAt(3) === 101/*e*/) {

Could just use protocol.indexOf('file') === 0, based on my testing that's about twice as fast if the protocol does match and equivalent if the first character doesn't match.

gideontee · 2018-02-12T23:09:21Z

Alright. I'll keep that in mind when I refactor. Thanks for the heads-up.

apapirovski · 2018-02-12T23:17:11Z

Looking it over again, I don't know how other collaborators will feel about refactoring that specific file. url.parse() is kind of being phased out and if it were being updated, it would probably need more significant work than just using constants. Most of the characters in there don't repeat as often either.

You could open an issue first to make sure there's support for it?

PR-URL: #18654 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

PR-URL: nodejs#18654 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. path Issues and PRs related to the path subsystem. labels Feb 8, 2018

BridgeAR reviewed Feb 8, 2018

View reviewed changes

richardlau requested changes Feb 8, 2018

View reviewed changes

apapirovski reviewed Feb 8, 2018

View reviewed changes

daynin force-pushed the refactor-path-module branch from 187f30d to 10de2d4 Compare February 8, 2018 21:26

BridgeAR approved these changes Feb 9, 2018

View reviewed changes

daynin force-pushed the refactor-path-module branch from 10de2d4 to d01f77b Compare February 9, 2018 16:03

richardlau approved these changes Feb 9, 2018

View reviewed changes

daynin force-pushed the refactor-path-module branch from d01f77b to 0889294 Compare February 9, 2018 16:06

BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed build Issues and PRs related to build files or the CI. labels Feb 9, 2018

apapirovski approved these changes Feb 9, 2018

View reviewed changes

devsnek previously requested changes Feb 9, 2018

View reviewed changes

kfarnung reviewed Feb 9, 2018

View reviewed changes

apapirovski reviewed Feb 9, 2018

View reviewed changes

path: replace "magic" numbers by readable constants

e551ccc

daynin force-pushed the refactor-path-module branch from 0889294 to e551ccc Compare February 9, 2018 19:54

BridgeAR reviewed Feb 9, 2018

View reviewed changes

jasnell approved these changes Feb 9, 2018

View reviewed changes

kfarnung approved these changes Feb 9, 2018

View reviewed changes

Fishrock123 approved these changes Feb 10, 2018

View reviewed changes

starkwang mentioned this pull request Feb 11, 2018

path: refactor to reduce duplicated codes #18594

Closed

3 tasks

BridgeAR closed this Feb 12, 2018

daynin deleted the refactor-path-module branch February 12, 2018 22:07

gideontee mentioned this pull request Feb 13, 2018

src: add ASCII table constants #18743

Closed

4 tasks

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added the dont-land-on-v6.x label Apr 13, 2018

MylesBorins added the dont-land-on-v8.x label Aug 7, 2018

This was referenced Mar 7, 2020

[Snyk] Security upgrade gatsby from 2.13.73 to 2.16.2 meghasfdc/node#4

Open

[Snyk] Security upgrade gatsby from 2.13.73 to 2.16.2 kissofire/node#3

Open

path: replace "magic" numbers by readable constants #18654

path: replace "magic" numbers by readable constants #18654

Conversation

daynin commented Feb 8, 2018

Checklist

Affected core subsystem(s)

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski left a comment • edited Loading

Choose a reason for hiding this comment

daynin commented Feb 9, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Feb 9, 2018

apapirovski left a comment

Choose a reason for hiding this comment

devsnek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daynin Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Feb 9, 2018

devsnek commented Feb 9, 2018 • edited Loading

apapirovski commented Feb 9, 2018

daynin commented Feb 9, 2018

BridgeAR commented Feb 9, 2018

apapirovski commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Feb 9, 2018

daynin commented Feb 10, 2018 • edited Loading

Fishrock123 left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 12, 2018

gideontee commented Feb 12, 2018

apapirovski commented Feb 12, 2018 • edited Loading

apapirovski commented Feb 12, 2018

gideontee commented Feb 12, 2018

apapirovski commented Feb 12, 2018

daynin commented Feb 12, 2018 • edited Loading

gideontee commented Feb 12, 2018

daynin commented Feb 12, 2018

gideontee commented Feb 12, 2018

apapirovski commented Feb 12, 2018

gideontee commented Feb 12, 2018

apapirovski commented Feb 12, 2018

apapirovski left a comment •

edited

Loading

daynin Feb 9, 2018 •

edited

Loading

devsnek commented Feb 9, 2018 •

edited

Loading

kfarnung Feb 9, 2018 •

edited

Loading

daynin commented Feb 10, 2018 •

edited

Loading

apapirovski commented Feb 12, 2018 •

edited

Loading

daynin commented Feb 12, 2018 •

edited

Loading