Skip to content

[Process] Fix double-fread() when reading unix pipes #19510

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

Merged
merged 1 commit into from
Aug 3, 2016

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Aug 2, 2016

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

While looking at the blackfire profile of a composer install, I was able to reduce the number of calls to fread from 90k to 60k using this patch (and from 60k to <1k with composer/composer#5569 but that's another story).

In fact, we should continue reading only if there might be something next, which won't be the case if the buffer has not been filled.

@nicolas-grekas nicolas-grekas merged commit ac17617 into symfony:2.7 Aug 3, 2016
nicolas-grekas added a commit that referenced this pull request Aug 3, 2016
…las-grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

[Process] Fix double-fread() when reading unix pipes

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

While looking at the blackfire profile of a `composer install`, I was able to reduce the number of calls to `fread` from 90k to 60k using this patch (and from 60k to <1k with composer/composer#5569 but that's another story).

In fact, we should continue reading only if there might be something next, which won"t be the case if the buffer has not been filled.

Commits
-------

ac17617 [Process] Fix double-fread() when reading unix pipes
@nicolas-grekas nicolas-grekas deleted the proc-less branch August 3, 2016 06:46
This was referenced Sep 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants