-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
optimizations #69
Comments
Huge 👍 to all of this @jonschlinkert. I look forward to discussing these updates in the future! |
I think this is a good idea. However, it would need to be a breaking release and we need to heavily test it against gulp use cases. As we found out with a different module of @jonschlinkert's (I think it was some globbing module), people are bundling gulp up with webpack which didn't take kindly to the lazy caching. |
I agree, it would be a breaking release.
Lol yeah, that's the one issue I mentioned. lazy-cache was used in micromatch. Right after micromatch was merged into glob-stream an issue was created on glob-stream because of a bug in webpack (related to getters) - which surfaced because of lazy-cache's use of getters. "perception is reality", so it looked like a micromatch and/or lazy-cache bug (and it was a regression in glob-stream either way), so we removed lazy-cache just to get rid of the regression and perception. Within 24-48 hrs we also found a couple of different solutions to the webpack bug to prevent future issues (webpack still has the bug, but lazy-cache is no longer effected by it). anyway I don't want to make lazy-cache a center point of this. I can just break this into in a few different prs so we can exclude the lazy-caching if you want. That part will take less than 5 minutes to add or remove, so we shouldn't lose sleep over it. I can just do it and we can compare before/after and discuss - or just not do it. |
I'd like to propose some optimizations that I would also be happy to implement after pr #67 is resolved.
In particular, I think if path caching, lazy-caching and getters/setters are implemented throughout the code it would result in pretty noticeable performance improvements. Just as importantly we can do so without complicating the code (it might even make the code cleaner).
Why?
Also, I implemented similar optimizations in verb and assemble and it reduced init time from 500-800 ms, to 150-200 ms. This makes the experience a lot nicer, and since init is faster it opens up the door for more interesting things (that might typically bog down the experience too much).
lazy-caching
Since liftoff is a command line application, it's a perfect use case for lazily evaluating requires. This way only libraries that are actually needed with be loaded upon invocation.
(fwiw, I've seen some developers have philosophical issues with this, but we use lazy-caching quite extensively and after 30+ million downloads of lazy-cache, we've had only one real issue related to webpack. And that was resolved months ago. Being a command line app, this would never be an issue here anyway)
Getters/setters
Anywhere an object is built up and paths are process, we have an opportunity to use getters/setters to evaluate and set the paths - similar to vinyl.
This way as the config/env is built up, whenever possible we would delay any path processing until it's needed.
Caching
This one probably wouldn't have much impact unless liftoff was used throughout the application lifecycle.
We would keep a cache of directories and files, and then whenever possible reuse the cached information instead of hitting the file system again.
The text was updated successfully, but these errors were encountered: