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

Redundant path-to-regexp computations in app.use #3065

Open
rohanpadhye opened this issue Aug 24, 2016 · 3 comments

Comments

@rohanpadhye
Copy link

@rohanpadhye rohanpadhye commented Aug 24, 2016

There seems to be a lot of redundant regexp construction when app.use is called with an array of middleware:

app.use(path, [f1, f2, f3])

A new layer is created for each middleware and the same path converted to a RegExp in the constructor of each layer. This effect gets exacerbated when path is itself an array of paths. For M paths and N middleware, there are O(MN) RegExp objects being created, though I feel only O(M) are really needed.

Here is a benchmark for a pathological case. When M = N = 1000, the call to app.use takes about 1.7 seconds on my machine.

Is there any reason why the regexp objects cannot be shared across all the layers? If all the layers share the regexp of the first layer, the same benchmark runs in only 7ms. The patch is quite hacky and hence I have not submitted a PR, but I would like to confirm if my assumptions are correct. Please let me know if I have misunderstood something.

@blakeembrey

This comment has been minimized.

Copy link
Member

@blakeembrey blakeembrey commented Aug 24, 2016

This is a really great point. I think I have an accidental patch (in the sense that I changed how the router path matching works) with pillarjs/router#29 because it's generating a matching function once and passing it down into each layer. /cc @dougwilson

@dougwilson

This comment has been minimized.

Copy link
Member

@dougwilson dougwilson commented Aug 24, 2016

Yea, and on top of that, a patch that would let people use whatever version of path-to-regexp or similar would also allow this to be fixed, but we just aren't there yet. @rohanpadhye I'm curious if this was encountered in a real-world application or simply a benchmarking thing.

@rohanpadhye

This comment has been minimized.

Copy link
Author

@rohanpadhye rohanpadhye commented Aug 25, 2016

@dougwilson Neither. I was using express as a test input for a dynamic analysis that I am developing using Jalangi to discover such issues automatically. I ran the unit tests in express through the analysis and it identified a possible source of redundancy in app.use.

Thanks for the confirmation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.