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
Improve performance of backend module with many log entries #198
Conversation
Classes/Controller/LogController.php
Outdated
'previousPage' => ($currentPage - 1) > 0 ? $currentPage - 1 : null, | ||
'nextPage' => $totalPages > $currentPage ? $currentPage + 1 : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null or 0 ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null is fine for me, as it is not page 0 but null as in »not defined«.
public function __construct(Filter $filter, LogRepository $logRepository) | ||
{ | ||
$this->from = new \DateTime(); | ||
$this->till = new \DateTime(); | ||
$count = $logEntries->count(); | ||
|
||
if ($count > 0) { | ||
$this->till->setTimestamp($logEntries->getFirst()->getTstamp()); | ||
$i = 1; | ||
if ($filter->getFrom() !== null) { | ||
$this->from->setTimestamp($filter->getFrom()); | ||
} else { | ||
$this->from->setTimestamp($logRepository->getFirstTimestampByFilter($filter)); | ||
} | ||
|
||
/** @var Log $logEntry */ | ||
foreach ($logEntries as $logEntry) { | ||
$this->traffic += $logEntry->getFileSize(); | ||
if ($i === $count) { | ||
$this->from->setTimestamp($logEntry->getTstamp()); | ||
} | ||
$i++; | ||
$this->till = new \DateTime(); | ||
if ($filter->getTill() !== null) { | ||
$this->till->setTimestamp($filter->getTill()); | ||
} else { | ||
if ($logRepository->getFirstTimestampByFilter($filter, true) > 0) { | ||
$this->till->setTimestamp($logRepository->getFirstTimestampByFilter($filter, true)); | ||
} | ||
} | ||
|
||
$this->traffic = $logRepository->getTrafficSumByFilter($filter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really make sense to start the entire query in the constructor? What if this information is not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is okay, as long as this class doesn't change it's purpose. Everything other method is a pure getter based on the class initialization.
->count('uid') | ||
->executeQuery() | ||
->fetchOne() ?? 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put $queryBuilder->...->fetchOne() ?? 0
into parentheses, so that the cast goes against the whole construct (not really necessary here, but looks better to me).
->orderBy('tstamp', $reverse ? 'DESC' : 'ASC') | ||
->executeQuery() | ||
->fetchOne() ?? 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
->selectLiteral('SUM(file_size) AS sum') | ||
->executeQuery() | ||
->fetchOne() ?? 0.0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
@@ -34,25 +33,25 @@ class Statistic | |||
*/ | |||
protected $till; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protected \DateTime $till;
(and the same for the lines above).
if ($filter->getTill() !== null) { | ||
$this->till->setTimestamp($filter->getTill()); | ||
} else { | ||
if ($logRepository->getFirstTimestampByFilter($filter, true) > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change
} else {
if ...
to
} elseif ...
No description provided.