FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

path: improve posixSplitPath performance by evanlucas · Pull Request #3034 · nodejs/node · GitHub

/ node Public

path: improve posixSplitPath performance - #3034

Merged
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:pathperf
Sep 25, 2015
ConversationCommits (1)ChecksFiles changed
Merged

path: improve posixSplitPath performance#3034
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:pathperf

Conversation

evanlucas commented Sep 23, 2015

Copy link
Copy Markdown
Contributor

Instead of slicing the first element off of the matches, manually build
the array. This improves performance of the following path.posix functions:

  • basename
    • path/basename.js type=win32 n=1000000: ./node: 847760 /usr/local/bin/node: 829710 .. 2.18%
    • path/basename.js type=posix n=1000000: ./node: 989290 /usr/local/bin/node: 847820 . 21.55%
  • extname
    • path/extname.js type=win32 n=1000000: ./node: 2385400 /usr/local/bin/node: 2451800 . -2.71%
    • path/extname.js type=posix n=1000000: ./node: 3379700 /usr/local/bin/node: 2113500 . 70.64%
  • dirname
    • path/dirname.js type=win32 n=1000000: ./node: 1699600 /usr/local/bin/node: 1706700 . -0.42%
    • path/dirname.js type=posix n=1000000: ./node: 2038700 /usr/local/bin/node: 1704300 . 21.99%
  • parse
    • path/parse.js type=win32 n=1000000: ./node: 1907900 /usr/local/bin/node: 1886300 .. 1.15%
    • path/parse.js type=posix n=1000000: ./node: 1923400 /usr/local/bin/node: 1616800 . 22.79%

evanlucas added the path Issues and PRs related to the path subsystem. label Sep 23, 2015

mscdex commented Sep 23, 2015

Copy link
Copy Markdown
Contributor

LGTM

evanlucas commented Sep 23, 2015

Copy link
Copy Markdown
Contributor Author

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

evanlucas commented Sep 23, 2015

Copy link
Copy Markdown
Contributor Author

meh, test-stringbytes-external on arm failed...as well as test-child-process-emfile on freebsd. I haven't seen that one fail before. Is that one known to be flaky at all?

thefourtheye commented Sep 24, 2015

Copy link
Copy Markdown
Contributor

@evanlucas They both are flaky, so don't worry about it :-)

thefourtheye reviewed Sep 24, 2015
View reviewed changes
Comment thread lib/path.js Outdated

thefourtheye Sep 24, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If there is no match, then matches will be null.

mscdex Sep 24, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That would have been the case before too, though. I'm not sure if it's possible to get no match with the regexp that's currently being used ...

alexlamsl commented Sep 24, 2015

Copy link
Copy Markdown

Just curious - why not Array.shift() and return?

evanlucas commented Sep 24, 2015

Copy link
Copy Markdown
Contributor Author

@alexlamsl Nice catch. I swear I had tried that before and it was a tiny bit slower than manually doing it, but that is not correct. It looks like shift is a little faster. Thanks!

evanlucas commented Sep 24, 2015

Copy link
Copy Markdown
Contributor Author

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

evanlucas commented Sep 24, 2015

Copy link
Copy Markdown
Contributor Author

CI looks happy (minus test-stringbytes-external.js). Will plan to land tomorrow if no one has any objections.

thefourtheye commented Sep 24, 2015

Copy link
Copy Markdown
Contributor

I still have my doubts about null, otherwise LGTM.

alexlamsl commented Sep 24, 2015

Copy link
Copy Markdown

@thefourtheye let me see if I can convince you...

     1       2                3.1           3.2          4
/^(\/?|)([\s\S]*?)((?:\.{1,2}|[^\/]+?|)(\.[^.\/]*|))(?:[\/]*)$/

Part 1 (\/?|) - can be empty
Part 2 ([\s\S]*?) - matches nothing or any character sequence
Part 3.1 (?:\.{1,2}|[^\/]+?|) - can be empty
Part 3.2 (\.[^.\/]*|) - can be empty
Part 4 (?:[\/]*) - matches nothing, or any number of /'s

So this pattern will always match.

path: improve posixSplitPath performance
b50e89e 
Instead of slicing the first element off of the matches, shift and then
return. This improves performance of the following path functions:

- basename: 18-20%
- extname: 60-70%
- dirname: 18-20%
- parse: 20-25%

PR-URL: nodejs#3034
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
evanlucas closed this Sep 25, 2015
evanlucas deleted the pathperf branch September 25, 2015 13:29
evanlucas merged commit b50e89e into nodejs:master Sep 25, 2015

evanlucas commented Sep 25, 2015

Copy link
Copy Markdown
Contributor Author

Thanks! Landed in b50e89e

evanlucas added a commit that referenced this pull request Sep 30, 2015
path: improve posixSplitPath performance
ac2bce0 
Instead of slicing the first element off of the matches, shift and then
return. This improves performance of the following path functions:

- basename: 18-20%
- extname: 60-70%
- dirname: 18-20%
- parse: 20-25%

PR-URL: #3034
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
This was referenced Sep 30, 2015
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

path Issues and PRs related to the path subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL