Skip to content

Commit

Permalink
Bug 1131737 - Expire history and favicons (#4068)
Browse files Browse the repository at this point in the history
* Bug 1131737 - Expire history and favicons

* Added test case for cleaning up old history/visits to StoragePerfTests.
  • Loading branch information
justindarc committed Jul 24, 2018
1 parent a4ff32c commit e613114
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 9 deletions.
51 changes: 44 additions & 7 deletions Storage/SQL/SQLiteHistoryRecommendations.swift
Expand Up @@ -171,15 +171,52 @@ extension SQLiteHistory: HistoryRecommendations {
return [(clearHighlightsQuery, nil), (sql, args)]
}

public func repopulate(invalidateTopSites shouldInvalidateTopSites: Bool, invalidateHighlights shouldInvalidateHighlights: Bool) -> Success {
var queries: [(String, Args?)] = []
if shouldInvalidateTopSites {
queries.append(contentsOf: self.refreshTopSitesQuery())
// Checks if there are more than 100k items in the `visits` table.
// This is used as an indicator that we should clean up old history.
func checkIfCleanupIsNeeded() -> Deferred<Maybe<Bool>> {
let sql = "SELECT COUNT(*) FROM \(TableVisits)"

This comment has been minimized.

Copy link
@rnewman

rnewman Jul 25, 2018

Contributor

No! @justindarc query history, not visits.

100,000 visits ain't that much. My desktop Places has 250K places and 450K visits.

return self.db.runQuery(sql, args: nil, factory: IntFactory) >>== { cursor in
guard let visitCount = cursor[0], visitCount > 100000 else {

This comment has been minimized.

Copy link
@rnewman

rnewman Jul 25, 2018

Contributor

Break out a constant!

This comment has been minimized.

Copy link
@rnewman

rnewman Jul 25, 2018

Contributor

You should also consider setting this to a much higher number until you have metrics. If you don't have metrics, you are not in a position to set this value.

return deferMaybe(false)
}

return deferMaybe(true)
}
if shouldInvalidateHighlights {
queries.append(contentsOf: self.repopulateHighlightsQuery())
}

// Deletes the oldest 50k items from the `visits` table and their
// corresponding items in the `history` table. This only gets run when
// there are more than 100k items in the `visits` table. Because of
// this, we may need to clean up several times until we've crossed
// below the 100k-item threshold.
private func cleanupOldHistory() -> [(String, Args?)] {
let visitsSQL = """
DELETE FROM \(TableVisits) WHERE id IN (
SELECT id FROM \(TableVisits) ORDER BY \(TableVisits).date ASC LIMIT 50000

This comment has been minimized.

Copy link
@rnewman

rnewman Jul 25, 2018

Contributor

Constant!

And where did this number come from?! 50,000 visits ain't much.

)
"""
let historySQL = """
DELETE FROM \(TableHistory) WHERE NOT EXISTS (
SELECT 1 FROM \(TableVisits) WHERE \(TableVisits).siteID = \(TableHistory).id
)
"""
return [(visitsSQL, nil), (historySQL, nil)]
}

public func repopulate(invalidateTopSites shouldInvalidateTopSites: Bool, invalidateHighlights shouldInvalidateHighlights: Bool) -> Success {
return checkIfCleanupIsNeeded() >>== { doCleanup in
var queries: [(String, Args?)] = []
if doCleanup {
queries.append(contentsOf: self.cleanupOldHistory())
}
if shouldInvalidateTopSites {
queries.append(contentsOf: self.refreshTopSitesQuery())
}
if shouldInvalidateHighlights {
queries.append(contentsOf: self.repopulateHighlightsQuery())
}
return self.db.run(queries)
}
return self.db.run(queries)
}

public func getRecentBookmarks(_ limit: Int = 3) -> Deferred<Maybe<Cursor<Site>>> {
Expand Down
29 changes: 27 additions & 2 deletions StoragePerfTests/StoragePerfTests.swift
Expand Up @@ -53,6 +53,31 @@ class TestSQLiteHistoryFrecencyPerf: XCTestCase {
}
}

class TestSQLiteHistoryRecommendationsPerf: XCTestCase {
func testCheckIfCleanupIsNeeded() {
let files = MockFiles()
let db = BrowserDB(filename: "browser.db", schema: BrowserSchema(), files: files)
let prefs = MockProfilePrefs()
let history = SQLiteHistory(db: db, prefs: prefs)

history.clearHistory().succeeded()
let doCleanup1 = history.checkIfCleanupIsNeeded().value.successValue!
XCTAssertFalse(doCleanup1, "We should not need to perform clean-up")

// Each history item inserted will produce 20 visits. Since clean-up is triggered
// once we exceed 100,000 visits, inserting 5,001 history items here will result in
// exactly 100,020 visits which should trigger a clean-up.
populateHistoryForFrecencyCalculations(history, siteCount: 5001)
let doCleanup2 = history.checkIfCleanupIsNeeded().value.successValue!
XCTAssertTrue(doCleanup2, "We should not need to perform clean-up")

// This should trigger the actual clean-up operation to happen.
history.repopulate(invalidateTopSites: true, invalidateHighlights: true).succeeded()
let doCleanup3 = history.checkIfCleanupIsNeeded().value.successValue!
XCTAssertFalse(doCleanup3, "We should not need to perform clean-up")
}
}

class TestSQLiteHistoryTopSitesCachePref: XCTestCase {
func testCachePerf() {
let files = MockFiles()
Expand Down Expand Up @@ -81,14 +106,14 @@ private enum VisitOrigin {
}

private func populateHistoryForFrecencyCalculations(_ history: SQLiteHistory, siteCount count: Int) {
for i in 0...count {
for i in 0..<count {
let site = Site(url: "http://s\(i)ite\(i)/foo", title: "A \(i)")
site.guid = "abc\(i)def"

let baseMillis: UInt64 = baseInstantInMillis - 20000
history.insertOrUpdatePlace(site.asPlace(), modified: baseMillis).succeeded()

for j in 0...20 {
for j in 1...20 {
let visitTime = advanceMicrosecondTimestamp(baseInstantInMicros, by: (1000000 * i) + (1000 * j))
addVisitForSite(site, intoHistory: history, from: .local, atTime: visitTime)
addVisitForSite(site, intoHistory: history, from: .remote, atTime: visitTime)
Expand Down

0 comments on commit e613114

Please sign in to comment.