Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Commit

Permalink
Fix #3171: High input lag when typing in url bar. (#3179)
Browse files Browse the repository at this point in the history
BraveCore `Bookmarkv2.byFrequency` runs on main thread,
and is not performant in general. Takes about 200ms per query.
This commit moves this operation to a background cancellable thread.
  • Loading branch information
iccub authored Jan 7, 2021
1 parent 2a0ebb2 commit 5a7d18e
Show file tree
Hide file tree
Showing 8 changed files with 76 additions and 140 deletions.
8 changes: 4 additions & 4 deletions Client.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
0A24F7F0233E9136004D2F3A /* ObjcExceptionBridging.framework in CopyFiles */ = {isa = PBXBuildFile; fileRef = 0AC0D399233E7CAF0091EFA5 /* ObjcExceptionBridging.framework */; settings = {ATTRIBUTES = (CodeSignOnCopy, RemoveHeadersOnCopy, ); }; };
0A27ABA123D1B9C400194921 /* BrandedImageCalloutState.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A27ABA023D1B9C400194921 /* BrandedImageCalloutState.swift */; };
0A2BD6092497780A00254494 /* libadblock.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 0A2BD6082497780A00254494 /* libadblock.a */; };
0A2F921C22146B7700304249 /* FrecencyQuery.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A2F921B22146B7700304249 /* FrecencyQuery.swift */; };
0A2FF33F24A4B75100F88F43 /* vpncheckmark.json in Resources */ = {isa = PBXBuildFile; fileRef = 0A2FF33E24A4B75000F88F43 /* vpncheckmark.json */; };
0A39D56D21930B89008B2772 /* ScriptOpener.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A39D56C21930B89008B2772 /* ScriptOpener.swift */; };
0A39FE902486604D00290ABC /* GRDSubscriberCredential.h in Headers */ = {isa = PBXBuildFile; fileRef = 0A39FE832486604100290ABC /* GRDSubscriberCredential.h */; };
Expand All @@ -60,6 +59,7 @@
0A3C789F23056C910022F6D8 /* OnboardingSearchEnginesView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A3C789E23056C910022F6D8 /* OnboardingSearchEnginesView.swift */; };
0A3C78A1230597DA0022F6D8 /* OnboardingShieldsViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A3C78A0230597DA0022F6D8 /* OnboardingShieldsViewController.swift */; };
0A3C78A3230597F10022F6D8 /* OnboardingShieldsView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A3C78A2230597F10022F6D8 /* OnboardingShieldsView.swift */; };
0A40450C25A47BD6009EC321 /* FrecencyQuery.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A40450B25A47BD6009EC321 /* FrecencyQuery.swift */; };
0A4214E921A6EBCF006B8E39 /* SafeBrowsingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A4214E821A6EBCF006B8E39 /* SafeBrowsingTests.swift */; };
0A4214FF21AC0D6C006B8E39 /* TimeExtensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A4214F621AC0AFF006B8E39 /* TimeExtensions.swift */; };
0A42150121AC0E8E006B8E39 /* TimeExtensionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A42150021AC0E8E006B8E39 /* TimeExtensionTests.swift */; };
Expand Down Expand Up @@ -1285,7 +1285,6 @@
0A27ABA023D1B9C400194921 /* BrandedImageCalloutState.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BrandedImageCalloutState.swift; sourceTree = "<group>"; };
0A2BD6082497780A00254494 /* libadblock.a */ = {isa = PBXFileReference; lastKnownFileType = archive.ar; path = libadblock.a; sourceTree = "<group>"; };
0A2D5DF623571F02001ED889 /* ko-KR */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = "ko-KR"; path = "ko-KR.lproj/Storage.strings"; sourceTree = "<group>"; };
0A2F921B22146B7700304249 /* FrecencyQuery.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FrecencyQuery.swift; sourceTree = "<group>"; };
0A2FF33E24A4B75000F88F43 /* vpncheckmark.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = vpncheckmark.json; sourceTree = "<group>"; };
0A38EA7F216532DB00142710 /* CRUDProtocolsTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CRUDProtocolsTests.swift; sourceTree = "<group>"; };
0A39D56C21930B89008B2772 /* ScriptOpener.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ScriptOpener.swift; sourceTree = "<group>"; };
Expand All @@ -1308,6 +1307,7 @@
0A3C789E23056C910022F6D8 /* OnboardingSearchEnginesView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OnboardingSearchEnginesView.swift; sourceTree = "<group>"; };
0A3C78A0230597DA0022F6D8 /* OnboardingShieldsViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OnboardingShieldsViewController.swift; sourceTree = "<group>"; };
0A3C78A2230597F10022F6D8 /* OnboardingShieldsView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OnboardingShieldsView.swift; sourceTree = "<group>"; };
0A40450B25A47BD6009EC321 /* FrecencyQuery.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FrecencyQuery.swift; sourceTree = "<group>"; };
0A4214E821A6EBCF006B8E39 /* SafeBrowsingTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SafeBrowsingTests.swift; sourceTree = "<group>"; };
0A4214F621AC0AFF006B8E39 /* TimeExtensions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TimeExtensions.swift; sourceTree = "<group>"; };
0A42150021AC0E8E006B8E39 /* TimeExtensionTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TimeExtensionTests.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -3945,7 +3945,6 @@
5D1DC54B20AE004600905E5A /* Syncable.swift */,
5D1DC54C20AE004600905E5A /* Model.xcdatamodeld */,
0A53F3E621E6560A0086E80C /* InMemoryDataController.swift */,
0A2F921B22146B7700304249 /* FrecencyQuery.swift */,
);
path = models;
sourceTree = "<group>";
Expand Down Expand Up @@ -4370,6 +4369,7 @@
D3B6923E1B9F9A58004B87A4 /* FindInPageHelper.swift */,
39F4C1092045DB2E00746155 /* FocusHelper.swift */,
D0E55C4E1FB4FD23006DC274 /* FormPostHelper.swift */,
0A40450B25A47BD6009EC321 /* FrecencyQuery.swift */,
39DD030C1CD53E1900BC09B3 /* HomePageHelper.swift */,
D3C3696D1CC6B78800348A61 /* LocalRequestHelper.swift */,
0BB5B30A1AC0AD1F0052877D /* LoginsHelper.swift */,
Expand Down Expand Up @@ -5860,7 +5860,6 @@
buildActionMask = 2147483647;
files = (
0AA029F8222E899400D6B535 /* Bookmark+Reorder.swift in Sources */,
0A2F921C22146B7700304249 /* FrecencyQuery.swift in Sources */,
5D1DC56E20AE005400905E5A /* SyncBookmark.swift in Sources */,
5D1DC55A20AE004600905E5A /* FaviconMO.swift in Sources */,
0AD4FEF0223AA45800E00C05 /* BraveCryptoJS.swift in Sources */,
Expand Down Expand Up @@ -6047,6 +6046,7 @@
D3C3696E1CC6B78800348A61 /* LocalRequestHelper.swift in Sources */,
2726637324981B600056CFE1 /* FeedSectionHeaderView.swift in Sources */,
27C461DE211B76500088A441 /* ShieldsView.swift in Sources */,
0A40450C25A47BD6009EC321 /* FrecencyQuery.swift in Sources */,
E4B423DD1ABA0318007E66C8 /* ReaderModeHandlers.swift in Sources */,
276A791024476FD200AC9446 /* StatsSectionProvider.swift in Sources */,
27FCA8E02447708300A8CA48 /* FavoritesSectionProvider.swift in Sources */,
Expand Down
5 changes: 4 additions & 1 deletion Client/Frontend/Browser/BrowserViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1295,8 +1295,11 @@ class BrowserViewController: UIViewController {
searchController.searchDelegate = self
searchController.profile = self.profile

searchLoader = SearchLoader(profile: profile, topToolbar: topToolbar)
searchLoader = SearchLoader()
searchLoader?.addListener(searchController)
searchLoader?.autocompleteSuggestionHandler = { [weak self] completion in
self?.topToolbar.setAutocompleteSuggestion(completion)
}

addChild(searchController)
view.addSubview(searchController.view)
Expand Down
40 changes: 40 additions & 0 deletions Client/Frontend/Browser/FrecencyQuery.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright 2020 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.

import Foundation
import Shared
import Storage
import Data

class FrecencyQuery {
private static let queue = DispatchQueue(label: "frecency-query-queue")
private static var cancellable: DispatchWorkItem?

public static func sitesByFrecency(containing query: String? = nil,
completion: @escaping (Set<Site>) -> Void) {
cancellable?.cancel()
cancellable = nil

// CoreData is fast, can be fetched on main thread. This also prevents threading problems.
let historySites = History.byFrecency(query: query)
.map { Site(url: $0.url ?? "", title: $0.title ?? "") }
cancellable = DispatchWorkItem {
// brave-core fetch can be slow over 200ms per call,
// a cancellable serial queue is used for it.
let bookmarkSites = Bookmarkv2.byFrequency(query: query)
.map { Site(url: $0.url ?? "", title: $0.title ?? "", bookmarked: true) }

let result = Set<Site>(historySites + bookmarkSites)

DispatchQueue.main.async {
completion(result)
}
}

if let task = cancellable {
queue.async(execute: task)
}
}
}
63 changes: 10 additions & 53 deletions Client/Frontend/Browser/Search/SearchLoader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,70 +2,26 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

import Foundation
import Shared
import Storage
import XCGLogger
import Data

private let log = Logger.browserLogger

// swiftlint:disable:next force_try
private let URLBeforePathRegex = try! NSRegularExpression(pattern: "^https?://([^/]+)/", options: [])

// TODO: Swift currently requires that classes extending generic classes must also be generic.
// This is a workaround until that requirement is fixed.
typealias SearchLoader = _SearchLoader<AnyObject, AnyObject>

/**
* Shared data source for the SearchViewController and the URLBar domain completion.
* Since both of these use the same SQL query, we can perform the query once and dispatch the results.
*/
class _SearchLoader<UnusedA, UnusedB>: Loader<[Site], SearchViewController> {
fileprivate let profile: Profile
fileprivate let topToolbar: TopToolbarView
fileprivate var inProgress: Cancellable?

init(profile: Profile, topToolbar: TopToolbarView) {
self.profile = profile
self.topToolbar = topToolbar

super.init()
}

// `weak` usage here allows deferred queue to be the owner. The deferred is always filled and this set to nil,
// this is defensive against any changes to queue (or cancellation) behaviour in future.
private weak var currentDbQuery: Cancellable?
/// Shared data source for the SearchViewController and the URLBar domain completion.
/// Since both of these use the same query, we can perform the query once and dispatch the results.
class SearchLoader: Loader<[Site], SearchViewController> {
var autocompleteSuggestionHandler: ((String) -> Void)?

var query: String = "" {
didSet {
guard let profile = self.profile as? BrowserProfile else {
assertionFailure("nil profile")
return
}

if query.isEmpty {
load([])
return
}

if let inProgress = inProgress {
inProgress.cancel()
self.inProgress = nil
}

let deferred = FrecencyQuery.sitesByFrecency(containing: query)
inProgress = deferred as? Cancellable

deferred.uponQueue(.main) { result in
defer {
self.inProgress = nil
}

// First, see if the query matches any URLs from the user's search history.
let bookmarks = Set<Site>(Bookmarkv2.byFrequency(query: self.query).map { Site(url: $0.url ?? "", title: $0.title ?? "", bookmarked: true) })
FrecencyQuery.sitesByFrecency(containing: query) { [weak self] result in
guard let self = self else { return }

self.load(Array(Set(result + bookmarks)))
self.load(Array(result))

// If the new search string is not longer than the previous
// we don't need to find an autocomplete suggestion.
Expand All @@ -75,7 +31,7 @@ class _SearchLoader<UnusedA, UnusedB>: Loader<[Site], SearchViewController> {

for site in result {
if let completion = self.completionForURL(site.url) {
self.topToolbar.setAutocompleteSuggestion(completion)
self.autocompleteSuggestionHandler?(completion)
return
}
}
Expand All @@ -87,7 +43,8 @@ class _SearchLoader<UnusedA, UnusedB>: Loader<[Site], SearchViewController> {
// Extract the pre-path substring from the URL. This should be more efficient than parsing via
// NSURL since we need to only look at the beginning of the string.
// Note that we won't match non-HTTP(S) URLs.
guard let match = URLBeforePathRegex.firstMatch(in: url, options: [], range: NSRange(location: 0, length: url.count)) else {
guard let urlBeforePathRegex = try? NSRegularExpression(pattern: "^https?://([^/]+)/", options: []),
let match = urlBeforePathRegex.firstMatch(in: url, options: [], range: NSRange(location: 0, length: url.count)) else {
return nil
}

Expand Down
27 changes: 5 additions & 22 deletions Data/models/Bookmark.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ import Foundation
import Shared
import Storage

public protocol WebsitePresentable {
var title: String? { get }
var url: String? { get }
}

private let log = Logger.browserLogger

public final class Bookmark: NSManagedObject, WebsitePresentable, Syncable, CRUD {
Expand Down Expand Up @@ -679,28 +684,6 @@ extension Bookmark: Comparable {
}
}

extension Bookmark: Frecencyable {
static func byFrecency(query: String? = nil,
context: NSManagedObjectContext = DataController.viewContext) -> [WebsitePresentable] {
let fetchRequest = NSFetchRequest<Bookmark>()
fetchRequest.fetchLimit = 5
fetchRequest.entity = Bookmark.entity(context: context)

var predicate = NSPredicate(format: "lastVisited > %@", History.thisWeek as CVarArg)
if let query = query {
predicate = NSPredicate(format: predicate.predicateFormat + " AND url CONTAINS %@", query)
}
fetchRequest.predicate = predicate

do {
return try context.fetch(fetchRequest)
} catch {
log.error(error)
}
return [Bookmark]()
}
}

extension Int {
func compare(_ against: Int) -> ComparisonResult {
if self > against { return .orderedDescending }
Expand Down
35 changes: 0 additions & 35 deletions Data/models/FrecencyQuery.swift

This file was deleted.

27 changes: 13 additions & 14 deletions Data/models/History.swift
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,19 @@ public final class History: NSManagedObject, WebsitePresentable, CRUD {

return try context.fetch(fetchRequest)
}

/// Returns a recently visites sites. Ran on main thread context
public static func byFrecency(query: String? = nil) -> [WebsitePresentable] {
let urlKeyPath = #keyPath(History.url)
let visitedOnKeyPath = #keyPath(History.visitedOn)

var predicate = NSPredicate(format: "\(visitedOnKeyPath) > %@", History.thisWeek as CVarArg)
if let query = query {
predicate = NSPredicate(format: predicate.predicateFormat + " AND \(urlKeyPath) CONTAINS %@", query)
}

return all(where: predicate, fetchLimit: 100, context: DataController.viewContext) ?? []
}
}

// MARK: - Internal implementations
Expand All @@ -139,17 +152,3 @@ extension History {
}
}

extension History: Frecencyable {
static func byFrecency(query: String? = nil,
context: NSManagedObjectContext = DataController.viewContext) -> [WebsitePresentable] {
let urlKeyPath = #keyPath(History.url)
let visitedOnKeyPath = #keyPath(History.visitedOn)

var predicate = NSPredicate(format: "\(visitedOnKeyPath) > %@", History.thisWeek as CVarArg)
if let query = query {
predicate = NSPredicate(format: predicate.predicateFormat + " AND \(urlKeyPath) CONTAINS %@", query)
}

return all(where: predicate, fetchLimit: 100, context: context) ?? []
}
}
11 changes: 0 additions & 11 deletions DataTests/BookmarkTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -604,17 +604,6 @@ class BookmarkTests: CoreDataTestCase {
XCTAssertNotNil(dict["objectData"])
}

func testFrecencyQuery() {
insertBookmarks(amount: 6)

let found = Bookmark.byFrecency(query: "brave")
// Query limit is 5
XCTAssertEqual(found.count, 5)

let notFound = Bookmark.byFrecency(query: "notfound")
XCTAssertEqual(notFound.count, 0)
}

// MARK: - Helpers

func testSyncOrderValidation() {
Expand Down

0 comments on commit 5a7d18e

Please sign in to comment.