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

optimize relativePath #126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davidmarkclements
Copy link

@davidmarkclements davidmarkclements commented Feb 13, 2017

The cached-path-relative module hits process.cwd() a lot, I know this might be an outlier but under certain condition its possible to end up with Error: ENFILE: file table overflow, uv_cwd.

cached-path-relative also calls bind on path.relative (for no apparent reason) - which is known to be slow.

Since module-deps (nor browserify) does a process.chdir (not has any need to) we can reduce the code down to a var declaration and a five line function.

@ghost
Copy link

ghost commented Feb 14, 2017

Thanks for digging into this. Can we pull @ashaffer into the discussion? The original patch is from browserify/browserify#1543

@ashaffer
Copy link
Member

ashaffer commented Feb 14, 2017

The cached-path-relative module hits process.cwd() a lot, I know this might be an outlier but under certain condition its possible to end up with Error: ENFILE: file table overflow, uv_cwd.

That's interesting, I didn't know about that. However, I originally didn't have that call inside there and then moved it in because its possible for the parent process to change directories on you, thus making your cache invalid.

Is there a way we could be notified of chdirs? I don't know if we can just ignore chdirs entirely, unfortunately. Browserify is async, and the working directory could be changed in the middle of a browserify. This would be kinda weird - but I can maybe see a plugin doing it for some odd reason. It's not obvious to me that this would be safe. Thoughts?

cached-path-relative also calls bind on path.relative (for no apparent reason) - which is known to be slow.

Good point. That is completely unnecessary, I have no memory of why that is in there. I'll fix that now.

@davidmarkclements
Copy link
Author

What if we exposed the ability to reset the cache to plugins? Then if a plugin hits this problem it can do its own check and clear?

@ghost
Copy link

ghost commented Feb 18, 2017

Could we pass the cwd as an argument so it can be done once at the start of the dependency process?

@ashaffer
Copy link
Member

@substack that seems like the right approach to me. It does seem possible it will break some people's existing (hacky) workflows, but that seems ultimately more correct.

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