Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Git pool #57

Merged
merged 3 commits into from
Jun 22, 2015
Merged

Git pool #57

merged 3 commits into from
Jun 22, 2015

Conversation

brendonparker
Copy link
Contributor

This addresses the github timeout errors that can occur on Windows platforms.
Fixed the git-pool branch to return a promise.
Added some "smarts" around the pool size, and whether or not to use the pool at all (doesn't seem to be an issue for non-windows platforms).

Should fix jspm/jspm-cli#675

@guybedford
Copy link
Member

Thanks so much for picking this up, it is really appreciated.

The only thing that worries me is that pooling git lookups will directly affect the install times. Because dependencies are discovered in lookups as git executions, a limit of two means that we can't discover the package tree depth in parallel, which is not ideal at all.

So as much as possible it would be good to limit the pool down to its necessary cases only.

I haven't personally noticed any issues on Windows 7 or 8 either with the pooling, so I think it may only be a Windows server issue? Perhaps we can restrict to only windows server in the pooling condition, and then widen the detection if other users report issues. I would even ideally link it directly to the version of Windows as well, as future versions of Windows server may not have the issue and get slowed down unnecessarily.

So perhaps if we can add os.type() or os.release() checking in there or some process.env check? Let me know if something along those lines might be suitable.

@brendonparker
Copy link
Contributor Author

I'm not sure how to differentiate between Windows 8 and Server 2012 with node.
I think the problem is definitely under the umbrella of Windows (with msysgit being the bottleneck - slowness issues here have popped up in numerous other projects based on what I've gathered). I didn't experience a timeout issue on my Win 7 laptop. However, it also has 8 logical cores and 16GB of ram. I think the problem is not beween the different releases of the the OS as much as the resources available. I hit this problem for when I was setting up a single core build machine. It just couldn't keep up with the flood of git processes, and eventually they'd time out.

All that to say, my take would be that any smarts around Windows 7/8 versus Server 2008/2012 wouldn't be helpful.

The only other thing I could think would be some sort of external configuration that determines the size of the pool, that could be tweaked per machine. I haven't spent enough time with the jspm code to know if this kind of configuration is already in place; where the right place for it to live.

@guybedford
Copy link
Member

Sure, thanks for the thoughtful feedback - sounds sensible. Having the pool determined by the number of processors as you are doing seems like this should allow performance to be fine on Windows desktops anyway.

Merging, will aim to release after a little testing this side.

guybedford added a commit that referenced this pull request Jun 22, 2015
@guybedford guybedford merged commit 1fc54ae into jspm:master Jun 22, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jspm install times out and 100% CPU usage
3 participants