Bookmark suggestions performance improvement (#799)

Task/Issue URL: https://app.asana.com/0/1199230911884351/1203228817591315/f Tech Design URL: CC: Description: This PR improves the performance of the bookmark suggestions feature. Previously, this function was iterating over all bookmark URLs, then mapping those to an array of bookmarks, before concatenating them all into one big array. This was done every single time you typed a character into the address bar, despite the result of the function being the same every single time. This now returns the existing sorted bookmarks array, doing no extra work.
duckduckgo · Oct 25, 2022 · e073a8c · e073a8c
1 parent 47e2d9f
commit e073a8c
Show file tree

Hide file tree

Showing 5 changed files with 18 additions and 14 deletions.
diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml
@@ -69,8 +69,8 @@ jobs:
       with:
         submodules: recursive
 
-    - name: Select Xcode 13.4
-      run: sudo xcode-select -s /Applications/Xcode_13.4.app/Contents/Developer
+    - name: Select Xcode 14.0
+      run: sudo xcode-select -s /Applications/Xcode_14.0.app/Contents/Developer
 
     - name: Build and test
       run: |

diff --git a/DuckDuckGo/Bookmarks/Model/BookmarkList.swift b/DuckDuckGo/Bookmarks/Model/BookmarkList.swift
@@ -17,17 +17,22 @@
 //
 
 import Foundation
+import BrowserServicesKit
 import os.log
 
 struct BookmarkList {
 
-    struct IdentifiableBookmark: Equatable {
+    struct IdentifiableBookmark: Equatable, BrowserServicesKit.Bookmark {
         let id: UUID
         let url: URL
+        let title: String
+        let isFavorite: Bool
 
         init(from bookmark: Bookmark) {
             self.id = bookmark.id
             self.url = bookmark.url
+            self.title = bookmark.title
+            self.isFavorite = bookmark.isFavorite
         }
     }
 
@@ -149,9 +154,8 @@ struct BookmarkList {
         return newBookmark
     }
 
-    func bookmarks() -> [Bookmark] {
-        let mappedBookmarks = allBookmarkURLsOrdered.compactMap { itemsDict[$0.url] }
-        return mappedBookmarks.reduce([], +)
+    func bookmarks() -> [IdentifiableBookmark] {
+        return allBookmarkURLsOrdered
     }
 
 }
diff --git a/DuckDuckGo/Suggestions/Model/SuggestionContainer.swift b/DuckDuckGo/Suggestions/Model/SuggestionContainer.swift
@@ -115,5 +115,3 @@ extension HistoryEntry: BrowserServicesKit.HistoryEntry {
     }
 
 }
-
-extension Bookmark: BrowserServicesKit.Bookmark {}
diff --git a/Unit Tests/Bookmarks/Model/BookmarkListTests.swift b/Unit Tests/Bookmarks/Model/BookmarkListTests.swift
@@ -30,7 +30,7 @@ final class BookmarkListTests: XCTestCase {
         bookmarkList.insert(bookmark)
 
         XCTAssert(bookmarkList.bookmarks().count == 1)
-        XCTAssert(bookmarkList.bookmarks().first == bookmark)
+        XCTAssert((bookmarkList.bookmarks()).first == bookmark.identifiableBookmark)
         XCTAssertNotNil(bookmarkList[bookmark.url])
     }
 
@@ -42,7 +42,7 @@ final class BookmarkListTests: XCTestCase {
         bookmarkList.insert(bookmark)
 
         XCTAssert(bookmarkList.bookmarks().count == 1)
-        XCTAssert(bookmarkList.bookmarks().first == bookmark)
+        XCTAssert(bookmarkList.bookmarks().first == bookmark.identifiableBookmark)
     }
 
     func testWhenBookmarkIsRemoved_ThenItIsNoLongerInList() {
@@ -53,7 +53,7 @@ final class BookmarkListTests: XCTestCase {
 
         bookmarkList.remove(bookmark)
 
-        XCTAssertFalse(bookmarkList.bookmarks().contains(bookmark))
+        XCTAssertFalse(bookmarkList.bookmarks().contains(bookmark.identifiableBookmark))
         XCTAssertNil(bookmarkList[bookmark.url])
     }
 
@@ -87,7 +87,7 @@ final class BookmarkListTests: XCTestCase {
         XCTAssert(bookmarkList[bookmark.url]?.isFavorite == bookmark.isFavorite)
         XCTAssert(bookmarkList[bookmark.url]?.title == bookmark.title)
         XCTAssert(bookmarkList.bookmarks().count == 1)
-        XCTAssert(bookmarkList.bookmarks().first == bookmark)
+        XCTAssert(bookmarkList.bookmarks().first == bookmark.identifiableBookmark)
         XCTAssertNotNil(bookmarkList[bookmark.url])
         XCTAssertNil(bookmarkList[unknownBookmark.url])
         XCTAssertNil(updateUrlResult)
@@ -141,4 +141,8 @@ fileprivate extension Bookmark {
                                               isFavorite: false,
                                               faviconManagement: FaviconManagerMock())
 
+    var identifiableBookmark: BookmarkList.IdentifiableBookmark {
+        return BookmarkList.IdentifiableBookmark(from: self)
+    }
+
 }
diff --git a/Unit Tests/Bookmarks/Model/LocalBookmarkManagerTests.swift b/Unit Tests/Bookmarks/Model/LocalBookmarkManagerTests.swift
@@ -147,8 +147,6 @@ final class LocalBookmarkManagerTests: XCTestCase {
         let (bookmarkManager, bookmarkStoreMock) = LocalBookmarkManager.aManager
         let bookmark = bookmarkManager.makeBookmark(for: URL.duckDuckGo, title: "Title", isFavorite: false)!
 
-        bookmark.isFavorite = !bookmark.isFavorite
-
         let newURL = URL.duckDuckGoAutocomplete
         let newBookmark = bookmarkManager.updateUrl(of: bookmark, to: newURL)