Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

path: refactor for performance and consistency #1778

Closed
wants to merge 2 commits into from
Closed

path: refactor for performance and consistency #1778

wants to merge 2 commits into from

Conversation

nwoltman
Copy link
Contributor

Convergence PR prompted by #35
Original PR: nodejs/node-v0.x-archive#9289

Original PR message (slightly modified):


Improve performance by:

  • Not leaking the arguments object in win32.join
  • Getting the last character of a string by index, instead of with .substr() or .slice()

Improve code consistency by:

  • Using [] instead of .charAt() where possible
  • Using a function declaration instead of a var declaration
  • Using .slice() with clearer arguments

Improve both by:

  • Making the reusable trimArray() function
  • Standardizing getting certain path statistics with the new win32StatPath() function

Benchmarks: (higher is better)

function (path.) current (ops/sec) this PR (ops/sec)
win32.resolve 315,019 399,543
win32.normalize 873,559 1,051,170
win32.isAbsolute 1,915,332 1,939,849
win32.join 452,879 815,033
win32.relative 153,821 219,928
win32.format 13,118,588 16,364,384
posix.relative 244,638 304,456
Benchmark code (gist)

@mscdex mscdex added the path Issues and PRs related to the path subsystem. label May 23, 2015
@@ -37,6 +37,29 @@ function normalizeArray(parts, allowAboveRoot) {
return res;
}

// returns an array with empty elements removed from either end of the input
Copy link
Contributor

Choose a reason for hiding this comment

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

First letter should be capitalized.

@silverwind
Copy link
Contributor

Are there any API changes? (seeing that the tests were modified)

@nwoltman
Copy link
Contributor Author

@silverwind I'm not sure if this counts as an API change, but as discussed here, if a user passes in an object where the dir property is null or undefined, these changes make it so that the function returns the correct value instead of incorrectly throwing.

@jbergstroem
Copy link
Member

I'm keen on getting the benchmark in (benchmarks/fs/) as well – but we haven't started using benchmark.js just quite yet. If you're keen, rewriting it slightly would be nice.

@nwoltman
Copy link
Contributor Author

@jbergstroem It looks like each path method would require it's own benchmark file. That's going to be a fair number of files so perhaps they should go in a new benchmark/path directory?
Then there's the question of whether or not the path.win32 and path.posix methods should be in their own files, or if they should be compared in the same file (similar to the Buffer/SlowBuffer comparison).

@ChALkeR
Copy link
Member

ChALkeR commented May 23, 2015

Looks like #1752 conflicts with this. I will remove the path.win32 part from #1752 once this gets merged.

Btw,

In path.win32.join this PR changes the stack of the TypeError('Arguments to path.join must be strings'), removing the top at f (path.js:190:13) and at Object.filter (native) so now it is similar to the stack in path.posix.join.

Is also valid for this PR.

@jasnell
Copy link
Member

jasnell commented May 27, 2015

LGTM

@ChALkeR
Copy link
Member

ChALkeR commented May 27, 2015

Why using slice instead of substr/substring for strings?

@nwoltman
Copy link
Contributor Author

@ChALkeR I haven't changed any code to use slice instead of substr/substring. If you're specifically referring to this line, yes I could have changed slice to substr, but instead I changed the input value to make the code cleaner and easier to read.

@nwoltman
Copy link
Contributor Author

nwoltman commented Jun 6, 2015

@jbergstroem I added some benchmarks. Do those look about right?

@jbergstroem
Copy link
Member

@woollybogger apologies for the late reply. LGTM. CI here: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/827/

@nwoltman
Copy link
Contributor Author

@jbergstroem A couple unrelated (and possibly flaky) tests failed. Does that matter?

@jbergstroem
Copy link
Member

@woollybogger no, that's something we're working on improving. I can't access the old results -- can you rebase on top of latest master so we can get a new run going?

You might want to see if anyone else wants to review before I/someone else look at landing it. @ChALkeR?

@nwoltman
Copy link
Contributor Author

Rebased

@jbergstroem
Copy link
Member

@jbergstroem
Copy link
Member

(never mind the windows timeouts)

@ChALkeR
Copy link
Member

ChALkeR commented Jun 17, 2015

The main part (lib/path.js) LGTM.

Minor thing: you replace single-char .substr() and .charAt() with [] in all places of path module, but this one:

posix.isAbsolute = function(path) {
  assertPath(path);
  return path.charAt(0) === '/';
};

Could you fix that for consistency?

@nwoltman
Copy link
Contributor Author

@ChALkeR Fixed.

@@ -37,6 +37,29 @@ function normalizeArray(parts, allowAboveRoot) {
return res;
}

// Returns an array with empty elements removed from either end of the input
// array or the original array if no elements need to be removed
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if returning the original array and a new array based on conditions is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a little helper function. There's no point in making a copy of an array when the original is all you need.

@nwoltman
Copy link
Contributor Author

bump 🚅

@brendanashworth brendanashworth added the benchmark Issues and PRs related to the benchmark subsystem. label Jun 26, 2015
isUnc: isUnc,
isAbsolute: isUnc || !!result[2], // UNC paths are always absolute
tail: result[3]
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use shorthand properties here:

return {
  device,
  isUnc,
  isAbsolute: isUnc || !!result[2], // UNC paths are always absolute
  tail: result[3]
};

@silverwind
Copy link
Contributor

LGTM, few nits and a question above.

Improve performance by:
+ Not leaking the `arguments` object!
+ Getting the last character of a string by index, instead of
  with `.substr()` or `.slice()`

Improve code consistency by:
+ Using `[]` instead of `.charAt()` where possible
+ Using a function declaration instead of a var declaration
+ Using `.slice()` with clearer arguments
+ Checking if `dir` is truthy in `win32.format`
  (added tests for this)

Improve both by:
+ Making the reusable `trimArray()` function
+ Standardizing getting certain path statistics with
  the new `win32StatPath()` function
Path functions being benchmarked are:
* format
* isAbsolute
* join
* normalize
* relative
* resolve
@nwoltman
Copy link
Contributor Author

nwoltman commented Jul 3, 2015

Updated

@silverwind
Copy link
Contributor

LGTM

silverwind pushed a commit that referenced this pull request Jul 4, 2015
Improve performance by:
+ Not leaking the `arguments` object!
+ Getting the last character of a string by index, instead of
  with `.substr()` or `.slice()`

Improve code consistency by:
+ Using `[]` instead of `.charAt()` where possible
+ Using a function declaration instead of a var declaration
+ Using `.slice()` with clearer arguments
+ Checking if `dir` is truthy in `win32.format`
  (added tests for this)

Improve both by:
+ Making the reusable `trimArray()` function
+ Standardizing getting certain path statistics with
  the new `win32StatPath()` function

PR-URL: #1778
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
silverwind pushed a commit that referenced this pull request Jul 4, 2015
Path functions being benchmarked are:
* format
* isAbsolute
* join
* normalize
* relative
* resolve

PR-URL: #1778
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@silverwind
Copy link
Contributor

Landed in bca53dc and 0d15161

@silverwind silverwind closed this Jul 4, 2015
@nwoltman nwoltman deleted the path branch July 8, 2015 03:36
mscdex pushed a commit to mscdex/io.js that referenced this pull request Jul 9, 2015
Improve performance by:
+ Not leaking the `arguments` object!
+ Getting the last character of a string by index, instead of
  with `.substr()` or `.slice()`

Improve code consistency by:
+ Using `[]` instead of `.charAt()` where possible
+ Using a function declaration instead of a var declaration
+ Using `.slice()` with clearer arguments
+ Checking if `dir` is truthy in `win32.format`
  (added tests for this)

Improve both by:
+ Making the reusable `trimArray()` function
+ Standardizing getting certain path statistics with
  the new `win32StatPath()` function

PR-URL: nodejs#1778
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
mscdex pushed a commit to mscdex/io.js that referenced this pull request Jul 9, 2015
Path functions being benchmarked are:
* format
* isAbsolute
* join
* normalize
* relative
* resolve

PR-URL: nodejs#1778
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants