diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ed6d0eb..f50e81d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Changed -* *Nothing* +* [#692](https://github.com/shlinkio/shlink/issues/692) Drastically improved performance when loading visits. Specially noticeable when loading big result sets. #### Deprecated diff --git a/data/migrations/Version20200503170404.php b/data/migrations/Version20200503170404.php new file mode 100644 index 00000000..a102c2c8 --- /dev/null +++ b/data/migrations/Version20200503170404.php @@ -0,0 +1,27 @@ +getTable('visits'); + $this->skipIf($visits->hasIndex(self::INDEX_NAME)); + $visits->addIndex(['date'], self::INDEX_NAME); + } + + public function down(Schema $schema): void + { + $visits = $schema->getTable('visits'); + $this->skipIf(! $visits->hasIndex(self::INDEX_NAME)); + $visits->dropIndex(self::INDEX_NAME); + } +} diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php index 803b9790..5143389b 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php @@ -32,6 +32,8 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->columnName('`date`') ->build(); + $builder->addIndex(['date'], 'IDX_visits_date'); + $builder->createField('remoteAddr', Types::STRING) ->columnName('remote_addr') ->length(Visitor::REMOTE_ADDRESS_MAX_LENGTH) diff --git a/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php b/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php index 247ea93e..6a42ecbc 100644 --- a/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php +++ b/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php @@ -15,6 +15,8 @@ class VisitsPaginatorAdapter implements AdapterInterface private ShortUrlIdentifier $identifier; private VisitsParams $params; + private ?int $count = null; + public function __construct( VisitRepositoryInterface $visitRepository, ShortUrlIdentifier $identifier, @@ -38,7 +40,17 @@ class VisitsPaginatorAdapter implements AdapterInterface public function count(): int { - return $this->visitRepository->countVisitsByShortCode( + // Since a new adapter instance is created every time visits are fetched, it is reasonably safe to internally + // cache the count value. + // The reason it is cached is because the Paginator is actually calling the method twice. + // An inconsistent value could be returned if between the first call and the second one, a new visit is created. + // However, it's almost instant, and then the adapter instance is discarded immediately after. + + if ($this->count !== null) { + return $this->count; + } + + return $this->count = $this->visitRepository->countVisitsByShortCode( $this->identifier->shortCode(), $this->identifier->domain(), $this->params->getDateRange(), diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 61b2afb8..547493c5 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -5,9 +5,17 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Repository; use Doctrine\ORM\EntityRepository; +use Doctrine\ORM\Query\ResultSetMappingBuilder; use Doctrine\ORM\QueryBuilder; +use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Entity\VisitLocation; + +use function preg_replace; + +use const PHP_INT_MAX; class VisitRepository extends EntityRepository implements VisitRepositoryInterface { @@ -81,24 +89,60 @@ class VisitRepository extends EntityRepository implements VisitRepositoryInterfa ?int $limit = null, ?int $offset = null ): array { - $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $domain, $dateRange); - $qb->select('v') - ->orderBy('v.date', 'DESC'); + /** + * @var QueryBuilder $qb + * @var ShortUrl|int $shortUrl + */ + [$qb, $shortUrl] = $this->createVisitsByShortCodeQueryBuilder($shortCode, $domain, $dateRange); + $qb->select('v.id') + ->orderBy('v.id', 'DESC') + // Falling back to values that will behave as no limit/offset, but will workaround MS SQL not allowing + // order on sub-queries without offset + ->setMaxResults($limit ?? PHP_INT_MAX) + ->setFirstResult($offset ?? 0); - if ($limit !== null) { - $qb->setMaxResults($limit); + // FIXME Crappy way to resolve the params into the query. Best option would be to inject the sub-query with + // placeholders and then pass params to the main query + $shortUrlId = $shortUrl instanceof ShortUrl ? $shortUrl->getId() : $shortUrl; + $subQuery = preg_replace('/\?/', $shortUrlId, $qb->getQuery()->getSQL(), 1); + if ($dateRange !== null && $dateRange->getStartDate() !== null) { + $subQuery = preg_replace( + '/\?/', + '\'' . $dateRange->getStartDate()->toDateTimeString() . '\'', + $subQuery, + 1, + ); } - if ($offset !== null) { - $qb->setFirstResult($offset); + if ($dateRange !== null && $dateRange->getEndDate() !== null) { + $subQuery = preg_replace('/\?/', '\'' . $dateRange->getEndDate()->toDateTimeString() . '\'', $subQuery, 1); } - return $qb->getQuery()->getResult(); + // A native query builder needs to be used here because DQL and ORM query builders do not accept + // sub-queries at "from" and "join" level. + // If no sub-query is used, then performance drops dramatically while the "offset" grows. + $nativeQb = $this->getEntityManager()->getConnection()->createQueryBuilder(); + $nativeQb->select('v.id AS visit_id', 'v.*', 'vl.*') + ->from('visits', 'v') + ->join('v', '(' . $subQuery . ')', 'sq', $nativeQb->expr()->eq('sq.id_0', 'v.id')) + ->leftJoin('v', 'visit_locations', 'vl', $nativeQb->expr()->eq('v.visit_location_id', 'vl.id')) + ->orderBy('v.id', 'DESC'); + + $rsm = new ResultSetMappingBuilder($this->getEntityManager()); + $rsm->addRootEntityFromClassMetadata(Visit::class, 'v', ['id' => 'visit_id']); + $rsm->addJoinedEntityFromClassMetadata(VisitLocation::class, 'vl', 'v', 'visitLocation', [ + 'id' => 'visit_location_id', + ]); + + $query = $this->getEntityManager()->createNativeQuery($nativeQb->getSQL(), $rsm); + + return $query->getResult(); } public function countVisitsByShortCode(string $shortCode, ?string $domain = null, ?DateRange $dateRange = null): int { - $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $domain, $dateRange); - $qb->select('COUNT(DISTINCT v.id)'); + /** @var QueryBuilder $qb */ + [$qb] = $this->createVisitsByShortCodeQueryBuilder($shortCode, $domain, $dateRange); + $qb->select('COUNT(v.id)'); return (int) $qb->getQuery()->getSingleScalarResult(); } @@ -107,32 +151,26 @@ class VisitRepository extends EntityRepository implements VisitRepositoryInterfa string $shortCode, ?string $domain, ?DateRange $dateRange - ): QueryBuilder { + ): array { + /** @var ShortUrlRepositoryInterface $shortUrlRepo */ + $shortUrlRepo = $this->getEntityManager()->getRepository(ShortUrl::class); + $shortUrl = $shortUrlRepo->findOne($shortCode, $domain) ?? -1; + $qb = $this->getEntityManager()->createQueryBuilder(); $qb->from(Visit::class, 'v') - ->join('v.shortUrl', 'su') - ->where($qb->expr()->eq('su.shortCode', ':shortCode')) - ->setParameter('shortCode', $shortCode); - - // Apply domain filtering - if ($domain !== null) { - $qb->join('su.domain', 'd') - ->andWhere($qb->expr()->eq('d.authority', ':domain')) - ->setParameter('domain', $domain); - } else { - $qb->andWhere($qb->expr()->isNull('su.domain')); - } + ->where($qb->expr()->eq('v.shortUrl', ':shortUrl')) + ->setParameter('shortUrl', $shortUrl); // Apply date range filtering if ($dateRange !== null && $dateRange->getStartDate() !== null) { $qb->andWhere($qb->expr()->gte('v.date', ':startDate')) - ->setParameter('startDate', $dateRange->getStartDate()); + ->setParameter('startDate', $dateRange->getStartDate(), ChronosDateTimeType::CHRONOS_DATETIME); } if ($dateRange !== null && $dateRange->getEndDate() !== null) { $qb->andWhere($qb->expr()->lte('v.date', ':endDate')) - ->setParameter('endDate', $dateRange->getEndDate()); + ->setParameter('endDate', $dateRange->getEndDate(), ChronosDateTimeType::CHRONOS_DATETIME); } - return $qb; + return [$qb, $shortUrl]; } }