Skip to content
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

Minor performance tweak for purge archive for deleted sites #14843

Merged
merged 7 commits into from Sep 3, 2019

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Sep 2, 2019

Not expecting a big change in performance here. But generally it's better to have two simple queries that can be fully read from index instead of joining the tables etc. The join should be straight forward though as well because it should be also created based on the index, however the two individual queries should be still faster.

Also another tweak is that for deleted sites we don't need to look at ts_archived. This actually does make things quite a bit faster.

@tsteur tsteur added the c: Performance For when we could improve the performance / speed of Matomo. label Sep 2, 2019
@tsteur tsteur added this to the 3.12.0 milestone Sep 2, 2019
@@ -116,8 +116,8 @@ public function test_purgeNoSiteArchives_PurgesAllNoSiteArchives()

//There are 5 rows for website #3. We leave the other two because they're before our purge threshold.
$deletedRowCount = $this->archivePurger->purgeDeletedSiteArchives($this->january);
$this->assertEquals(3 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount);
self::$fixture->assertArchivesDoNotExist(array(3, 7, 10), $this->january);
$this->assertEquals(5 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount);
Copy link
Member Author

Choose a reason for hiding this comment

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

We are now expecting more results as for deleted sites we don't need to look at ts_archived.

@tsteur tsteur added the Needs Review PRs that need a code review label Sep 3, 2019

$sql = "SELECT DISTINCT idarchive FROM " . $archiveTableName . " WHERE idsite IN (".Common::getSqlStringFieldsArray($deletedSites).")";

$rows = Db::getReader()->fetchAll($sql, $deletedSites);
Copy link
Member

Choose a reason for hiding this comment

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

If there are hundreds or thousands of deleted sites, will this be a problem? (since we're using placeholders)

Copy link
Member Author

Choose a reason for hiding this comment

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

I reckon it's unlikely this happens but will change it indeed 👍 doesn't hurt

Copy link
Member Author

Choose a reason for hiding this comment

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

Will also apply this to the deleteArchiveIds method that is in the same model

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 👍

@diosmosis diosmosis merged commit 9b3a14f into 3.x-dev Sep 3, 2019
@diosmosis diosmosis deleted the purgesitesarchives branch September 3, 2019 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Performance For when we could improve the performance / speed of Matomo. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants