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

[Packager] Ever since GA, Packager has been *really slow* for the first time. #341

Closed
jaygarcia opened this issue Mar 27, 2015 · 28 comments
Closed
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@jaygarcia
Copy link
Contributor

I only noticed this since GA. Am i alone in this?

[10:06:27] <START> request:/Examples/UIExplorer/UIExplorerApp.includeRequire.runModule.bundle?dev=true
[10:06:27] <START> find dependencies
[10:06:44] <END>   find dependencies(16238ms)
[10:06:44] <START> transform
[10:06:44] <END>   transform(476ms)
[10:06:44] <END>   request:/Examples/UIExplorer/UIExplorerApp.includeRequire.runModule.bundle?dev=true(16747ms)
@vjeux
Copy link
Contributor

vjeux commented Mar 27, 2015

What is GA?

@jaygarcia
Copy link
Contributor Author

General Availability. :)

JG

301.785.6030 :: @moduscreate

:: sent from my mobile device ::

On Mar 27, 2015, at 12:04, Christopher Chedeau notifications@github.com wrote:

What is GA?


Reply to this email directly or view it on GitHub.

@amasad
Copy link
Contributor

amasad commented Mar 27, 2015

One thing that has happened is I moved the packager's node_modules to the root. Making them crawlable by the packager which is probably slowing down things. This was to fix an urgent issue #225
We should make the packager a stand alone node package.

@amasad
Copy link
Contributor

amasad commented Mar 27, 2015

Actually just tried this on a couple of machines and it seems to be running ok, it's not taking more than 5 seconds. Can you try restarting your computer?

@jaygarcia
Copy link
Contributor Author

That's my point. 5 seconds on a cold start for packager is painfully slow. It wasn't this way before :(

JG

:: sent from my mobile device ::

On Mar 27, 2015, at 13:19, Amjad Masad notifications@github.com wrote:

Actually just tried this on a couple of machines and it seems to be running ok, it's not taking more than 5 seconds. Can you try restarting your computer?


Reply to this email directly or view it on GitHub.

@amasad
Copy link
Contributor

amasad commented Mar 27, 2015

Yes, we should fix it.

@sahrens
Copy link
Contributor

sahrens commented Mar 28, 2015

Agreed we can and should make it faster, but I'm curious: how often and for
what reasons do you do cold starts of the packager?

On Fri, Mar 27, 2015 at 10:54 AM, Amjad Masad notifications@github.com
wrote:

Yes, we should fix it.


Reply to this email directly or view it on GitHub
#341 (comment)
.

@jaygarcia
Copy link
Contributor Author

Not often :). I just happened to notice it after a kernel panic (debugging core audio implementation).

This also surfaced another strange thing: while the first cold-boot request is made to the packager, hit cmd+r. That induces a second request, causing your app to render twice.

This is most noticeable in "SampleApp"

JG

301.785.6030 :: @moduscreate

:: sent from my mobile device ::

On Mar 27, 2015, at 21:57, Spencer Ahrens notifications@github.com wrote:

Agreed we can and should make it faster, but I'm curious: how often and for
what reasons do you do cold starts of the packager?

On Fri, Mar 27, 2015 at 10:54 AM, Amjad Masad notifications@github.com
wrote:

Yes, we should fix it.


Reply to this email directly or view it on GitHub
#341 (comment)
.


Reply to this email directly or view it on GitHub.

@sahrens
Copy link
Contributor

sahrens commented Mar 29, 2015

I think the render twice issue has been fixed - does it still repro for you on master?

-Spencer

On Mar 28, 2015, at 5:37 AM, Jay Garcia notifications@github.com wrote:

Not often :). I just happened to notice it after a kernel panic (debugging core audio implementation).

This also surfaced another strange thing: while the first cold-boot request is made to the packager, hit cmd+r. That induces a second request, causing your app to render twice.

This is most noticeable in "SampleApp"

JG

301.785.6030 :: @moduscreate

:: sent from my mobile device ::

On Mar 27, 2015, at 21:57, Spencer Ahrens notifications@github.com wrote:

Agreed we can and should make it faster, but I'm curious: how often and for
what reasons do you do cold starts of the packager?

On Fri, Mar 27, 2015 at 10:54 AM, Amjad Masad notifications@github.com
wrote:

Yes, we should fix it.


Reply to this email directly or view it on GitHub
#341 (comment)
.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub.

@jaygarcia
Copy link
Contributor Author

Did pull this AM and if timed right, I can make > 2 show. I also discovered if i am an ass and hammer CMD + R, I can make it cause an exception. This second case is a much lower priority ;)

dups

@amasad amasad added the packager label Apr 1, 2015
@amasad
Copy link
Contributor

amasad commented Apr 1, 2015

@jaygarcia can you open another issue for the render twice?

@ide
Copy link
Contributor

ide commented Apr 3, 2015

The packager has been really slow after setting a couple of root directories even when it's hot. When I make a small change to one file and load the bundle in my browser, it takes about 6-7 seconds.

My hypothesis is that it's scanning deep into node_modules to see if there are any Haste directives (@amasad - would you know if this is the case?). I have some big devDependencies (gulp, coffeescript, etc.) and when I rm -rf node_modules I can now load a new bundle in less than a second. Could the packager stop traversing down the file system when it sees node_modules?

@joewood
Copy link

joewood commented Apr 3, 2015

You could try adding the dev dependency paths to the blacklist (react-native/packager/blacklist.js - sharedBlacklist). In fact, the packager could check the root package.json for devDependencies, assuming these are just tools.

@joewood
Copy link

joewood commented Apr 3, 2015

That, actually, probably won't help. The better fix would be to add a more elaborate glob pattern in the react-packager/src/service/index.js that lists the individual dirs ignoring the node_modules devDependencies.

@ide
Copy link
Contributor

ide commented Apr 3, 2015

Is there ever a need to scan node_modules for @providesModule? I thought about it a bit and the only case I could think of was for react-native to be able to pull in some helper modules from react-tools. Either node_modules/react-tools could be whitelisted, or those files in question could just ship with react-native proper (they may already, I haven't checked recently).

@sophiebits
Copy link
Contributor

Just for react-tools, as far as I know. We're trying to do work to get away from @providesModule for that too.

@amasad
Copy link
Contributor

amasad commented Apr 3, 2015

@ide Good point, although Internally, I think we have some modules living in node_modules with @providesModules. But we can make this a setting. We still need to read the directories to build the dependency graph but should be much faster than reading the files to look for providesModules.
(If we used watchman for the dir reading it should become super fast)

@amasad
Copy link
Contributor

amasad commented Apr 3, 2015

@ide btw, did you update recently? The bluebird promise patch should get you 30% speed improvement.

@ide
Copy link
Contributor

ide commented Apr 3, 2015

@amasad I'm using up to 08b48ca which is fairly recent.

Some setting to limit searching under node_modules sounds right (probably a whitelist, with support for "*" if FB needs it). Node/CommonJS-style path resolution still needs to search in node_modules but it should be much faster as you mentioned. It's just the Haste parsing that I think could be worth pruning.

@amasad
Copy link
Contributor

amasad commented Apr 4, 2015

I was mistaken that we didn't need to read those files, since the way the packager works now is that it builds up the graph ahead of time we still need to read the files to parse out the requires. Saving the docblock parsing isn't that big of a win. On a 14 second graph build time it saves 2 seconds. I'm going to try using watchman for the directory reading and file stating. I believe that'll have a bigger impact.

@amasad
Copy link
Contributor

amasad commented Apr 4, 2015

Additionally, caching the graph is not out of the question here but I always try to keep it as the last resort.

@ide
Copy link
Contributor

ide commented Apr 4, 2015

It might end up involving a bunch of work but I imagine lazily building the graph would help a lot here and not involve caching (good call). Ex: since my root file (ExampleApp.js) doesn't transitively require node_modules/gulp, the packager never need to scan those files. I could also try moving my devDependencies into another directory for now, just to hide them from the packager.

@amasad
Copy link
Contributor

amasad commented Apr 4, 2015

The node_modules part can be lazily constructed, however, haste modules cannot because the modules are not tied to the filesystem location. I think having the graph be half lazy / half eager would be more complex than caching. We have a cache module for the transformer and it's pretty stable, I'll see if it can be easily reused for this.

Btw, I just reread your earlier comment. I had missed that this was slow for you even after start up time.

When I make a small change to one file and load the bundle in my browser, it takes about 6-7 seconds.

Ooh, this shouldn't be the case at all. No recrawl happens when you change a file it should just get reprocess the module that changed and rebuild the package with everything else from cache. Can you double check the numbers? Maybe post the logs from your terminal

@ide
Copy link
Contributor

ide commented Apr 4, 2015

It's fast for me now actually. I'm not sure what changed, other than today's commits (only c32d9bc#diff-e5979e1e4c8039af168a3114bcbac5ffL67 looks related...).

@amasad
Copy link
Contributor

amasad commented Apr 5, 2015

@ide good. I still want to make sure startup time is better but, I think, it's not as important as the change/reload cycle being sub-second.

@jsierles
Copy link
Contributor

@jaygarcia how's the packager working for you now?

@brentvatne brentvatne changed the title Ever since GA, Packager has been *really slow* for the first time. [Packager] Ever since GA, Packager has been *really slow* for the first time. May 31, 2015
@jaygarcia
Copy link
Contributor Author

@jsierles DUDE! such a huge difference!!

React packager ready.

[10:46:03 AM] request:/jsx/index.ios.bundle
[10:46:03 AM] find dependencies
[10:46:03 AM] find dependencies (26ms)
[10:46:03 AM] transform
[10:46:04 AM] transform (50ms)
[10:46:04 AM] request:/jsx/index.ios.bundle (95ms)

Thanks :)

@amasad
Copy link
Contributor

amasad commented Jun 2, 2015

Next sync a speed up will land which will make node_modules dependency reading lazy, i.e. unless you require a node module, that file will not be read.

@facebook facebook locked as resolved and limited conversation to collaborators Jun 1, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

9 participants