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

Parallel downloader #5293

Closed
wants to merge 16 commits into from
Closed

Parallel downloader #5293

wants to merge 16 commits into from

Conversation

@hirak
Copy link
Contributor

hirak commented May 6, 2016

Parallel Prefetcher

porting from https://github.com/hirak/prestissimo

  • Fetching packages in parallel at Composer\Installer::doInstall()
  • This is NOT replacement of Composer\Util\RemoteFilesystem.
    • repository.json download in serial
  • If !extension_loaded('curl'), then fallback to RemoteFilesystem
  • If it fails to download, then fallback to RemoteFilesystem. (do not retry / do not show any prompt)

See also

#2696
#3930 (comment)

Question

github-token, gitlab-token options are still alive?

if (isset($options['github-token'])) {

if (isset($options['gitlab-token'])) {

I can't find any documentation about that options. In this P-R, I didn't implement it yet.

porting from hirak/prestissimo

public static function generateUserAgent()
{
static $ua;

This comment has been minimized.

Copy link
@staabm

staabm May 6, 2016

Contributor

Are all this caches really required?

This comment has been minimized.

Copy link
@staabm

staabm May 6, 2016

Contributor

Looks like some mirco ootimizing

This comment has been minimized.

Copy link
@hirak

hirak May 6, 2016

Author Contributor

Because User-Agent is never-changed string in Composer process. But it was off the subject(parallel downloading).
Should I remove it for simplicity ?

This comment has been minimized.

Copy link
@staabm

staabm May 6, 2016

Contributor

If it doesnt improve by a measurable amount of time I would drop it

@staabm
Copy link
Contributor

staabm commented May 6, 2016

Any numbers how much faster downloading gets with this PR?

/** @var resource<stream<plainfile>> */
protected $fp;

protected $success = false;

This comment has been minimized.

Copy link
@stof

stof May 6, 2016

Contributor

I suggest using private visibility for all of them. It makes maintaining backward compatibility much easier (as anything private is outside the surface concerned with BC, while protected stuff is inside it due to inheritance)

This comment has been minimized.

Copy link
@hirak

hirak May 6, 2016

Author Contributor

👍 OK

hirak added 4 commits May 6, 2016
@hirak
Copy link
Contributor Author

hirak commented May 7, 2016

@staabm It is a difficult question. Because it depends geolocation, composer command, etc..

For example, I benchmarked by create-project laravel/laravel.

San Francisco

  • composer-stable (1.0.3)
    [167.5MB/149.39s] Memory usage: 167.55MB (peak: 218.73MB), time: 149.39s
  • prestissimo-patch
    [168.4MB/97.77s] Memory usage: 168.39MB (peak: 215.27MB), time: 97.77s

Singapore

  • composer-stable (1.0.3)
    [167.6MB/338.51s] Memory usage: 167.56MB (peak: 218.76MB), time: 338.51s
  • prestissimo-patch
    [168.4MB/156.43s] Memory usage: 168.38MB (peak: 215.26MB), time: 156.43s

Details below
https://gist.github.com/hirak/fb720bb716c19e08a0ccb139f4ac8d81

@hirak
Copy link
Contributor Author

hirak commented May 7, 2016

Test failure is not my fault.
Pull Requested.
Drop test dependency on http://www.example.com #5294

private static function ifOr($str, $pre = '', $post = '')
{
if ($str) {
return "$pre$str$post";

This comment has been minimized.

Copy link
@stloyd

stloyd May 7, 2016

Contributor
return $pre . $str . $post;
}
}

private static $NSS_CIPHERS = array(

This comment has been minimized.

Copy link
@stloyd

stloyd May 7, 2016

Contributor

This should be defined before methods.

@hirak
Copy link
Contributor Author

hirak commented May 7, 2016

I built composer.phar for trial.
Release prestissimo-patch-preview · hirak/composer

if ($this->user) {
$user = $this->user;
$user .= self::ifOr($this->pass, ':');
$url .= "$user@";

This comment has been minimized.

Copy link
@stloyd

stloyd May 7, 2016

Contributor
$url .= $user . '@';
foreach (array('http', 'https') as $scheme) {
if ($this->scheme === $scheme) {
$label = $scheme . '_proxy';
foreach (array($label, strtoupper($label)) as $l) {

This comment has been minimized.

Copy link
@stloyd

stloyd May 7, 2016

Contributor

Loop inside loop looks like overkill, it's small but IMO two ifs would be more clear.

@hirak hirak mentioned this pull request May 7, 2016
6 of 6 tasks complete
$this->permanent = $permanent;

// for PHP<5.5 @see getFinishedResults()
$this->blackhole = fopen('php://memory', 'wb');

This comment has been minimized.

Copy link
@hirak

hirak May 10, 2016

Author Contributor

$this->blackhole = tmpfile(); or $this->blackhole = fopen('php://temp', 'wb');

I think 'php://temp' is better.
http://stackoverflow.com/questions/6841854/how-to-do-curl-put-requests-with-a-php-memory-file-handle

curl_setopt($ch, CURLOPT_FILE, $this->blackhole); //release file pointer
$index = (int)$ch;
$request = $this->runningRequests[$index];
if (CURLE_OK === $errno && !$error && (!preg_match('/^http/', $info['url']) || 200 === $info['http_code'])) {

This comment has been minimized.

Copy link
@stloyd

stloyd May 10, 2016

Contributor

Why not using strpos() instead of preg_match()?

false === strpos($info['url'], 'http')

This comment has been minimized.

Copy link
@hirak

hirak May 15, 2016

Author Contributor

I think substr() is the best choice for performance. I'll rewrite.

if (!file_exists($targetdir)) {
if (!mkdir($targetdir, 0766, true)) {
throw new FetchException(
"The file could not be written to $fileName."

This comment has been minimized.

Copy link
@stloyd

stloyd May 10, 2016

Contributor

Shouldn't you mention also the directory? And to match other parts of code this should be string concat.

hirak added 2 commits May 15, 2016
}
} while ($remains > 0);

return compact('successCnt', 'failureCnt', 'urls');

This comment has been minimized.

Copy link
@staabm

staabm May 15, 2016

Contributor

compact is not opimizable by php runtimes, better use the longer and more explicit form

$this->permanent = $permanent;

// for PHP<5.5 @see getFinishedResults()
$this->blackhole = fopen('php://temp', 'wb');

This comment has been minimized.

Copy link
@staabm

staabm May 15, 2016

Contributor

How much data will be written into this stream? php://temp will use a file and gets therefore slow if it is more than 2MB by default.

We could use php://memory which is always stored in memory.

See http://php.net/manual/en/wrappers.php.php for details

This comment has been minimized.

Copy link
@hirak

hirak May 15, 2016

Author Contributor

blackhole is never written in.
The purpose of blackhole is to release a file resource temporary.
https://github.com/composer/composer/pull/5293/files#diff-d0769a23664b99a74bc1abfef231cf87R152

CURLOPT_FILE needs FILE* castable resource. fopen('php://memory') is not compatible resource, some errors reported.
https://github.com/php/php-src/blob/PHP-5.3.29/ext/curl/interface.c#L1904
hirak/prestissimo#93

As far as I know, fopen('php://temp') is the best choice.

This comment has been minimized.

Copy link
@staabm

staabm May 15, 2016

Contributor

Which file resource does it release? One allocated by php-src?

@Seldaek Seldaek modified the milestones: 1.4, 1.3 Nov 6, 2016
@Seldaek Seldaek modified the milestones: 1.4, 2.0 Mar 7, 2017
@hboomsma
Copy link

hboomsma commented Oct 2, 2017

gitlab-token is alive, we are currently using it.

@CDRO
Copy link

CDRO commented Nov 13, 2017

Any news on this?

@m1guelpf
Copy link

m1guelpf commented Dec 11, 2017

@hirak Updates?

@LKaemmerling
Copy link

LKaemmerling commented Dec 19, 2017

@Seldaek are there any Planes to merge this? I Install this Plugin in every Environment we Develop or deploy and IT Speeds Up our deployment process 50%

@AbdelkaderBah
Copy link

AbdelkaderBah commented Dec 25, 2017

This will help a lot.

@joshuaadickerson
Copy link

joshuaadickerson commented Jan 26, 2018

Please merge. We're seeing major gains with parallel composer install/update.

@pedrofurtado
Copy link

pedrofurtado commented Feb 3, 2018

Any news about this issue?

@AbdelkaderBah
Copy link

AbdelkaderBah commented Feb 9, 2018

Hope you guys give a look for this pull, its been months I'm waiting this feature

@rakshazi
Copy link

rakshazi commented Feb 16, 2018

Still waiting for it :(

@staabm
Copy link
Contributor

staabm commented Feb 16, 2018

please stop spamming this topic with useless comments. jordi is aware that a lot of people would love this feature, but it is not top priority.

everybody can use the plugin in the meantime.

@rtuin rtuin mentioned this pull request Nov 9, 2018
@Seldaek Seldaek mentioned this pull request Jan 18, 2019
@Seldaek
Copy link
Member

Seldaek commented Jan 30, 2019

Closed by #7904 which adds curl-multi support for the upcoming 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.