Make cli archiving faster if only one concurrent request per website is configured #18157
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsteur
commented
Oct 14, 2021
| @@ -162,48 +165,67 @@ private function start($piwikUrls) | |||
|
|
|||
| if ($shouldStart === Request::ABORT) { | |||
| // output is needed to ensure same order of url to response | |||
| $output = new Output($cmdId); | |||
| $output = new StaticOutput($cmdId); | |||
There was a problem hiding this comment.
was actually not needed to write this to disc, instead we can store this in memory
tsteur
commented
Oct 14, 2021
| } else { | ||
| $output = new StaticOutput($cmdId); |
There was a problem hiding this comment.
same here, there was actually never a need to write this to disc, now that we have StaticOutput we don't need to use the regular output anymore (which should be named ideally like FileOutput but wasn't sure if it's used anywhere else)
tsteur
commented
Oct 14, 2021
| @@ -415,7 +452,9 @@ private function requestUrls(array $piwikUrls) | |||
| $elapsed = time() - $startTime; | |||
| $timeToWait = $this->getTimeToWaitBeforeNextCheck($elapsed); | |||
|
|
|||
| usleep($timeToWait); | |||
| if (!count($this->processes)) { | |||
| usleep($timeToWait); | |||
There was a problem hiding this comment.
when we use HTTP or syncCli we will now no longer need to wait as we'll know we have the result already at this point and the command is finished
tsteur
added
the
c: Performance
sgiehl
approved these changes
Oct 18, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR is based on #18152 which we'll want to merge first.
Previously, we were executing all cli commands asynchronous. This comes with a big overhead that we need to check the list of processes constantly, need to work with pid files, write output to disk, etc. On one of our systems we noticed this can lead to an overhead of close to 40% CPU on the archiving. It's probably bit over-reported but it had definitely a big impact and it's maybe more realistically maybe 10-20%. However, because a lot of the time is archiving spent is waiting for the dedicated DB the CPU increase percentage spent on this might be higher after all.
When
core:archiveruns with--concurrent-requests-per-website=1then there's actually no need to run a command asynchronous. Instead we could simply run the command synchronous meaning we won't need to check constantly if the process is still running and then sleep for a bit and check again. Instead we will be notified immediately when the command is finished. Also we 100% accurately notice immediately if command is aborted, avoid writing result to disk, using pids etc.This would not be a massive improvement for a regular system but for some systems where there are heaps of archivers launching constantly this can make quite an improvement.
Technically, this way we could in the future even support more systems possibly using CLI archiving as we wouldn't need
awkandpscommand to work anymore etc. However, changing that be a bit more complicated and it's impact is limited because it's only the case when using--concurrent-requests-per-website=1when the default value is 3. So by default, nobody will benefit from this improvement unless that--concurrent-requests-per-website=1is set. I wouldn't advice setting--concurrent-requests-per-website=1for this performance improvement when previously running multiple in parallel. It be mostly a nice performance benefit if already using--concurrent-requests-per-website=1for various reasons anyway.Added some tests for the
StaticOutputclass. There are actually already tests for CliMulti when using CLI and when there is only one concurrent archiver so didn't add any other ones.Review