Fire Performance improvement; Async/await APIs #380

mallexxx · 2021-12-20T11:24:39Z

Task/Issue URL: https://app.asana.com/0/1199237043630360/1201439804724065
Tech Design URL: https://app.asana.com/0/1199237043630360/1201473146283276
CC: @tomasstrba

Description:
The PR fixes Fire performance by removing Web Cache Files in background

Steps to test this PR:

Validate fire works and clears data
Validate Fire Performance improvement

Testing checklist:

Test with Release configuration

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

bstandaert-ddg · 2021-12-22T05:36:49Z

@mallexxx I tested this by using the fire button in a few different scenarios and observing the contents of ~/Library/Caches/com.duckduckgo.macos.browser.debug, and I'm seeing some unexpected behavior:

Clearing data for a specific site is clearing all data instead. Example: I visit duckduckgo.com; my WebKit directory is 2.6 MB. Then I visit github.com; now my WebKit directory is 11.2 MB. Then I use the fire button, and deselect DDG (so only github is selected), but the entire WebKit directory is erased and contains only empty folders.
Using the fire button more than once has unexpected behavior:

First I visit a few websites, and my WebKit directory grows in size as I do.
I use the fire button once, and I get an empty WebKit directory.
Then I visit some sites again, and the WebKit directory grows in size again.
Then I use the fire button a second time. This time the WebKit directory is deleted entirely (along with fsCachedData).
If I go visit some websites now, the WebKit directory isn't recreated.
If I restart the browser and then visit a site, the WebKit directory gets created again.

Sites still load correctly in the second-to-last step, but perhaps the disk cache isn't being used in that case?

Also, I think this is a preexisting issue, but the cache directory has a set of 3 files (Cache.*) that don't get cleared by the fire button. I looked through the contents of those files, and I'm seeing URLs and pieces of requests for autocomplete requests (containing the terms I typed in the searchbar) and favicon URLs for the domains I visited.

bstandaert-ddg · 2021-12-22T05:54:43Z

Made a new task for the last part: https://app.asana.com/0/1199178362774117/1201549708930601/f

Also, this was all tested on Monterey.

bstandaert-ddg · 2021-12-22T06:04:43Z

I tried to replicate the performance improvement, but after visiting a hundred or so pages, the original version still completes within a few seconds. Is there a good way to replicate the conditions in which it's slow, or do I just need a larger history set?

mallexxx · 2021-12-22T10:21:18Z

@bstandaert-ddg, nice catches!

It was always clearing the caches before, only Cookies are fireproof right now and no filtering of caches is done
I've added clearing of Cache.* files and recreating WebKit directory so it is reused after the second burn

tomasstrba · 2021-12-22T21:01:05Z

Updating my Xcode, probably won't review this evening but definitely tomorrow.

bstandaert-ddg · 2021-12-22T23:22:43Z

@mallexxx Thanks! The behavior of the webkit directory seems correct now.

After I use the fire button, the Cache.* files aren't recreated when I type something in the address bar - I have to restart the browser first, and then search for something to get them to show up again. Address bar autocomplete still works correctly after using the fire button, but are the requests not being cached at that point?

tomasstrba · 2021-12-23T21:33:52Z

Ben, thanks for your testing, it is such a huge help! 🥇Aaddress bar suggestions are completely independent and not related to WebKit framework. We fetch our own DDG service, local history and bookmarks.

bstandaert-ddg · 2021-12-23T21:43:05Z

@tomasstrba Right, sorry, should clarify more - the autocomplete API responses are cached in the Cache.db file, which Alexey added deletion of in this PR (but which I agree isn't from Webkit). For example, if I open the browser with an empty data directory and search for "coffee", and then inspect the Cache.db file:

ddg@Bens-MacBook-Pro ~ % sqlite3
SQLite version 3.36.0 2021-06-18 18:58:49
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite> .open /Users/ddg/Library/Caches/com.duckduckgo.macos.browser/Cache.db 
sqlite> select * from cfurl_cache_response;
1|0|-5633176981449468330|0|https://improving.duckduckgo.com/t/ml_mac_app-launch_as-nondefault_first-in-a-day?appVersion=0.18.1|2021-12-23 21:39:06|
2|0|2229491188499263403|0|https://improving.duckduckgo.com/t/ml_mac_launch-timing?appVersion=0.18.1&duration=1.5|2021-12-23 21:39:07|
3|0|-5105212876823782820|0|https://duckduckgo.com/ac/?q=co|2021-12-23 21:39:10|
4|0|-6075741280270610708|0|https://duckduckgo.com/ac/?q=c|2021-12-23 21:39:10|
5|0|4064392822393613702|0|https://duckduckgo.com/ac/?q=cof|2021-12-23 21:39:10|
6|0|-410894281|0|https://improving.duckduckgo.com/t/m_mac_suggestions-displayed_no-bookmarks_no-favorites_no-history-entry?appVersion=0.18.1|2021-12-23 21:39:10|
7|0|-8635798911630085762|0|https://duckduckgo.com/ac/?q=coff|2021-12-23 21:39:10|
8|0|8956837284803241424|0|https://duckduckgo.com/ac/?q=coffe|2021-12-23 21:39:10|
9|0|8740292940288447142|0|https://duckduckgo.com/ac/?q=coffee|2021-12-23 21:39:10|

And the response bodies are in another table of the same file:

sqlite> select * from cfurl_cache_receiver_data;
1|0|GIF89a
2|0|GIF89a
3|0|[{"phrase":"cox webmail"},{"phrase":"webmail.cox.net"},{"phrase":"costco"},{"phrase":"comcast email"},{"phrase":"cox"},{"phrase":"cool math games"},{"phrase":"covid testing near me"},{"phrase":"coinbase"},{"phrase":"commnet"}]
4|0|[{"phrase":"cnn"},{"phrase":"www.cnn.com"},{"phrase":"calculator"},{"phrase":"chrome"},{"phrase":"craigslist"},{"phrase":"chase"},{"phrase":"costco"},{"phrase":"capital one"},{"phrase":"cvs"}]
5|0|[{"phrase":"coffee"},{"phrase":"coffin"},{"phrase":"coffee makers"},{"phrase":"coffee near me"},{"phrase":"coffee shops near me"},{"phrase":"coffee tables"},{"phrase":"coffee mugs"},{"phrase":"coffin dance"}]
6|0|GIF89a
7|0|[{"phrase":"coffee"},{"phrase":"coffee near me"},{"phrase":"coffin"},{"phrase":"coffee makers"},{"phrase":"coffee grinder"},{"phrase":"coffee tables"},{"phrase":"coffee mugs"},{"phrase":"coffin dance"}]
8|0|[{"phrase":"coffee"},{"phrase":"coffee near me"},{"phrase":"coffee makers"},{"phrase":"coffee shops near me"},{"phrase":"coffee grinder"},{"phrase":"coffee tables"},{"phrase":"coffee mugs"},{"phrase":"coffee meets bagel"},{"phrase":"coffeemeetsbagel.com"}]
9|0|[{"phrase":"coffee"},{"phrase":"coffee makers"},{"phrase":"coffee near me"},{"phrase":"coffee tables"},{"phrase":"coffee shops near me"},{"phrase":"coffee mugs"},{"phrase":"coffee grinder"},{"phrase":"coffee beans"},{"phrase":"www.coffeebean.com"}]

tomasstrba · 2021-12-25T12:55:11Z

Ben, thanks for the clarification. Good catch! I have noticed these logs after usage of fire button that are probably related. Requests may not be cached because file descriptors to cache files are invalidated. And only the restart of the browser opens cache files correctly again.

2021-12-25 13:43:19.829845+0100 DuckDuckGo[63233:630905] [logging] BUG IN CLIENT OF libsqlite3.dylib: database integrity compromised by API violation: vnode renamed while in use: /private/var/folders/8b/9kzczvb10rb7_4phdsctqrd00000gn/T/TemporaryItems/86F48992-4C96-4553-8BAD-59172DC6ADD2/Cache.db-shm
2021-12-25 13:43:19.829907+0100 DuckDuckGo[63233:630905] [logging] invalidated open fd: 23 (0x20)
2021-12-25 13:43:19.830079+0100 DuckDuckGo[63233:630905] [logging] BUG IN CLIENT OF libsqlite3.dylib: database integrity compromised by API violation: vnode renamed while in use: /private/var/folders/8b/9kzczvb10rb7_4phdsctqrd00000gn/T/TemporaryItems/86F48992-4C96-4553-8BAD-59172DC6ADD2/Cache.db-wal
2021-12-25 13:43:19.830134+0100 DuckDuckGo[63233:630905] [logging] invalidated open fd: 22 (0x20)
2021-12-25 13:43:19.830493+0100 DuckDuckGo[63233:631016] [logging] BUG IN CLIENT OF libsqlite3.dylib: database integrity compromised by API violation: vnode renamed while in use: /private/var/folders/8b/9kzczvb10rb7_4phdsctqrd00000gn/T/TemporaryItems/86F48992-4C96-4553-8BAD-59172DC6ADD2/Cache.db
2021-12-25 13:43:19.830541+0100 DuckDuckGo[63233:631016] [logging] invalidated open fd: 21 (0x20)
2021-12-25 13:43:23.992687+0100 DuckDuckGo[63233:630942] [logging] BUG IN CLIENT OF libsqlite3.dylib: database integrity compromised by API violation: vnode unlinked while in use: /private/var/folders/8b/9kzczvb10rb7_4phdsctqrd00000gn/T/TemporaryItems/86F48992-4C96-4553-8BAD-59172DC6ADD2/Cache.db-shm
2021-12-25 13:43:23.992749+0100 DuckDuckGo[63233:630942] [logging] invalidated open fd: 23 (0x11)
2021-12-25 13:43:23.992937+0100 DuckDuckGo[63233:630942] [logging] BUG IN CLIENT OF libsqlite3.dylib: database integrity compromised by API violation: vnode unlinked while in use: /private/var/folders/8b/9kzczvb10rb7_4phdsctqrd00000gn/T/TemporaryItems/86F48992-4C96-4553-8BAD-59172DC6ADD2/Cache.db-wal
2021-12-25 13:43:23.993004+0100 DuckDuckGo[63233:630942] [logging] invalidated open fd: 22 (0x11)
2021-12-25 13:43:23.993313+0100 DuckDuckGo[63233:630942] [logging] BUG IN CLIENT OF libsqlite3.dylib: database integrity compromised by API violation: vnode unlinked while in use: /private/var/folders/8b/9kzczvb10rb7_4phdsctqrd00000gn/T/TemporaryItems/86F48992-4C96-4553-8BAD-59172DC6ADD2/Cache.db
2021-12-25 13:43:23.993342+0100 DuckDuckGo[63233:630942] [logging] invalidated open fd: 21 (0x11)
2021-12-25 13:43:38.813261+0100 DuckDuckGo[63233:631114] [logging] volume does not support data protection, stripping SQLITE_OPEN_FILEPROTECTION_* flags
2021-12-25 13:43:38.813887+0100 DuckDuckGo[63233:631114] [logging] volume does not support data protection, stripping SQLITE_OPEN_FILEPROTECTION_* flags

tomasstrba

LGTM! Code looks great! So simplified with the new API 🥇

The only blocker is the issue Ben found. I hope we don't need to restart the browser after usage of fire button. Which processes are writing to those files? Can we restart just them specifically if it is not the main process?

tomasstrba · 2021-12-25T12:34:11Z

DuckDuckGo/BrowserTab/Services/WebsiteDataStore.swift

-    func clear(completion: @escaping () -> Void) {
+    func clear(domains: Set<String>? = nil) async {
+        // first cleanup ~/Library/Caches
+        await self.clearFileCache()


wow, how clean are these lines 💯 thans for introducing the usage of async await. 🥇

tomasstrba · 2021-12-25T12:40:09Z

DuckDuckGo/Fire/Model/Fire.swift

@@ -57,9 +57,10 @@ final class Fire {
        let burningDomains = domains.union(wwwDomains)

        group.enter()
-        burnWebCache(domains: burningDomains, completion: {
+        Task {


👍 can't wait to use this too

tomasstrba

LGTM! 👍

tomasstrba · 2022-01-11T12:34:27Z

DuckDuckGo/Common/Extensions/URLSessionExtension.swift

+    private static var defaultCallbackQueue: OperationQueue = {
+        let queue = OperationQueue()
+        queue.name = "APIRequest default callback queue"
+        queue.qualityOfService = .utility


Please, do you think we could raise QoS to .userInitiated? Utility is performing work which the user is unlikely to be immediately waiting for the results, for example periodic content updates or bulk file operations. Not sure if we feel the difference, but let's ask for better resources if we can

bstandaert-ddg · 2022-01-11T18:56:29Z

The behavior of clearing cache.db looks good to me now, at least once the favicon service get updated as well.

I'm not sure if this is worth holding this PR up for, but I just noticed that if a tab is writing to the cache directory at the same time as you press the fire button, it's possible for the cache directory to not be empty afterwards. You can reproduce by going to twitter.com and pressing the fire button within a few seconds of when the page loads:

Screen.Recording.2022-01-11.at.12.51.06.PM.mov

Task/Issue URL: https://app.asana.com/0/1201037661562251/1204717237528130/f BSK PR: duckduckgo/BrowserServicesKit#380 Description: Moves storing the password salt to a background queue on creation. (This is a BSK fix)

* develop: (246 commits) Bump version to 1.44.0 (31) Update embedded files Updates the way we store the secure vault encryption keys. (#1256) ask fireproofing off by default (#1269) Set version to 1.43.1. fix(duck-player): capture the correct url before a click occurs (#1268) Remove Rogue "Passwords" button that appears when clicking the password field in the Save Creds popover (#1250) Reattach orphaned bookmarks to root folder when reordering (#1260) Update API usage and trigger sync when interacting with bookmarks (#1255) Move the exti event to first engagement (#1244) Drop support for regexes in url parameter stripping (#1220) bump bsk (#1261) Add Re-enable .superceded Notification action (#1240) Fix pop up window button issues and hide NetP in pop up windows (#1257) resolve export bookmarks warning (#1254) History Integration Tests fixed (#1253) Add item to keychain in the background #380 (#1251) fix sysex reinstall codesign issue (#1236) macOS in-context signup updates (#1209) Fixes the format in a log call (#1249) ...

mallexxx added 2 commits December 20, 2021 17:22

Fire Performance improvement; Async/await APIs

1cb2ba0

fix cache files paths

3069d4a

mallexxx requested a review from tomasstrba December 20, 2021 11:57

mallexxx assigned tomasstrba Dec 20, 2021

Recreated Caches WebKit directory after removal

aa920b2

tomasstrba approved these changes Dec 25, 2021

View reviewed changes

rm random files

20bac4b

mallexxx assigned mallexxx and unassigned tomasstrba Jan 10, 2022

mallexxx added 2 commits January 11, 2022 13:43

Merge branch 'develop' into alex/fire-performance

2918d38

Use ephemeral URLSession for API requests

c278a9b

tomasstrba assigned tomasstrba and unassigned mallexxx Jan 11, 2022

tomasstrba approved these changes Jan 11, 2022

View reviewed changes

tomasstrba assigned mallexxx and unassigned tomasstrba Jan 11, 2022

tomasstrba mentioned this pull request Jan 11, 2022

Favicons #388

Merged

mallexxx added 3 commits January 12, 2022 12:40

Fix Common/Extensions ordering

354c89b

fix test

fda87ad

fix dependency: set lottie version

a7dac38

Merge remote-tracking branch 'origin/develop' into alex/fire-performance

c29707d

mallexxx merged commit 88c49d4 into develop Jan 12, 2022

mallexxx deleted the alex/fire-performance branch January 12, 2022 13:11

ayoy unassigned mallexxx Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fire Performance improvement; Async/await APIs #380

Fire Performance improvement; Async/await APIs #380

mallexxx commented Dec 20, 2021

bstandaert-ddg commented Dec 22, 2021

bstandaert-ddg commented Dec 22, 2021

bstandaert-ddg commented Dec 22, 2021

mallexxx commented Dec 22, 2021 •

edited

Loading

tomasstrba commented Dec 22, 2021

bstandaert-ddg commented Dec 22, 2021

tomasstrba commented Dec 23, 2021

bstandaert-ddg commented Dec 23, 2021

tomasstrba commented Dec 25, 2021 •

edited

Loading

tomasstrba left a comment

tomasstrba Dec 25, 2021

tomasstrba Dec 25, 2021

tomasstrba left a comment

tomasstrba Jan 11, 2022

bstandaert-ddg commented Jan 11, 2022

Fire Performance improvement; Async/await APIs #380

Fire Performance improvement; Async/await APIs #380

Conversation

mallexxx commented Dec 20, 2021

Internal references:

bstandaert-ddg commented Dec 22, 2021

bstandaert-ddg commented Dec 22, 2021

bstandaert-ddg commented Dec 22, 2021

mallexxx commented Dec 22, 2021 • edited Loading

tomasstrba commented Dec 22, 2021

bstandaert-ddg commented Dec 22, 2021

tomasstrba commented Dec 23, 2021

bstandaert-ddg commented Dec 23, 2021

tomasstrba commented Dec 25, 2021 • edited Loading

tomasstrba left a comment

Choose a reason for hiding this comment

tomasstrba Dec 25, 2021

Choose a reason for hiding this comment

tomasstrba Dec 25, 2021

Choose a reason for hiding this comment

tomasstrba left a comment

Choose a reason for hiding this comment

tomasstrba Jan 11, 2022

Choose a reason for hiding this comment

bstandaert-ddg commented Jan 11, 2022

mallexxx commented Dec 22, 2021 •

edited

Loading

tomasstrba commented Dec 25, 2021 •

edited

Loading