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

Create an optimistic mkdirp #9

Merged
merged 6 commits into from
May 22, 2019
Merged

Conversation

jclem
Copy link
Contributor

@jclem jclem commented May 22, 2019

This implements a simpler, optimistic form of mkdirp. The code is significantly easier to read and will be faster in cases where most of the given path already exists.

@jclem jclem requested a review from bryanmacfarlane May 22, 2019 17:37
@damccorm
Copy link
Contributor

damccorm commented May 22, 2019

There's 2 orthogonal issues here:

  1. Changing from iterative to recursive implementation. That seems fine to me, I don't see much advantage though.

  2. Changing to the optimistic approach. I don't actually think its going to make things faster though. Looking only at more time consuming calls:

In the case the folder already exists:

The current implementaion stats it, then checks if its a directory. The proposed implementation tries to make the folder, fails, stats the folder, then checks if its a directory. So the proposed implementation is marginally slower.

In the case the folder doesn't exist, but the path to the folder exists:

The current implementation tries to stat it, fails, stats its parent, checks if its parent is a directory, and then calls mkdir on the new folder. The proposed implementation just creates the new folder. So the proposed implementation is marginally faster.

In the case multiple folders need to be created

The current implementation tries to stat each missing folder/check if they're directories and eventually calls mkdir on each missing folder. The proposed implementation tries to call mkdir on each missing folder, fails, and then eventually calls it successfully on each missing folder. At best, the proposed implementation seems to be marginally slower here, I would guess this is actually a more significant slow down though.

So to summarize:
Folder already exists: current implementation is faster
Only one folder needs to be created: proposed implementation is faster
Multiple folders need to be created: current implementation is faster

So I don't see a ton of advantage here.

EDIT: Took out "It seems to me like the multiple folders need to be created case is the most useful here as well, otherwise you can easily just use fs." Because as I thought about it that's not really true, the advantage of mkdirp is that you don't need to know how many folders you have to create (could be 0, 1, or many).

@jclem
Copy link
Contributor Author

jclem commented May 22, 2019

@damccorm It’s more about what I saw as some nice opportunities for maintainability improvements here, more than performance.

packages/io/src/io-util.ts Outdated Show resolved Hide resolved
packages/io/src/io-util.ts Outdated Show resolved Hide resolved
@damccorm
Copy link
Contributor

damccorm commented May 22, 2019

It’s more about what I saw as some nice opportunities for maintainability improvements here, more than performance.

Cool, that sounds good. I agree its more readable and perf won't be hugely impacted, just wanted to make sure we weren't making these changes with the expectation that it would be.

@damccorm
Copy link
Contributor

LGTM

@damccorm damccorm merged commit 60a8dfb into features/io May 22, 2019
@damccorm damccorm deleted the features/io-optimistic-mkdirp branch May 22, 2019 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants