Skip to content

Run one file at a time #10

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
Dec 29, 2015
Merged

Run one file at a time #10

merged 1 commit into from
Dec 29, 2015

Conversation

gdiggs
Copy link
Contributor

@gdiggs gdiggs commented Dec 29, 2015

In my testing, this implode call was taking a really long time to run,
and phpmd itself seemed faster when only given one file at a time.

@codeclimate/review

);
foreach ($files as &$file) {
$phpmd->processFiles(
$file,//implode(",", $files),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you're keeping the commented out bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! Fixed.

In my testing, this `implode` call was taking a really long time to run,
and phpmd itself seemed faster when only given one file at a time.
@wfleming
Copy link
Contributor

LGTM. I wonder how this was interacting with the fork-daemon stuff in engine.php that is supposed to provide concurrency.

@gdiggs
Copy link
Contributor Author

gdiggs commented Dec 29, 2015

@wfleming As i watched the output (with lots of extra logging), each worker would hang at the implode call for a bit.

@wfleming
Copy link
Contributor

@gordondiggs Oh, I totally believe you. I'm just unclear on how the fork-daemon is working or what it's doing. I thought the fork-daemon basically dispatched individual files to workers, but this makes it look more like it does some kind of batching? Maybe that's what this line controls.

@gdiggs
Copy link
Contributor Author

gdiggs commented Dec 29, 2015

@wfleming I think that is correct

gdiggs added a commit that referenced this pull request Dec 29, 2015
@gdiggs gdiggs merged commit b2e6fd7 into master Dec 29, 2015
@gdiggs gdiggs deleted the gd-batch branch December 29, 2015 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants