1905068 - Use a cached list of all available temporary origins for pattern-based clearing of origins

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, task, P3)

Description

[Jan Varga [:janv]]

Pattern-based clearing of origins can be slow on some machines, especially those with older disks and many origin directories.
Bug 1867997 (patch https://phabricator.services.mozilla.com/D198990) introduces a cached list of all available temporary origins which can be used for pattern-based clearing of origins as well.
This should have notable effect on the clearing operation especially in cases when there are no matches for given pattern and the cost of IO-based traversal is much higher than the cost of moving origin directories to the "to-be-deleted" folder.

Comment 1

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #0)

Bug 1867997 (patch https://phabricator.services.mozilla.com/D198990) introduces a cached list of all available temporary origins which can be used for pattern-based clearing of origins as well.

The patch landed some time ago, but we should wait a bit to make sure any follow-up regressions are fixed before changing the clearing to use the cached list of origins.

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1905068 - Use a cached list of all available temporary origins for pattern-based clearing of origins; r=#dom-storage

Comment 3

[Jan Varga [:janv]]

This can be now postponed given we see reduced quota manager shutdown time and even shutdown hangs after bug 1933053.

Comment 4

[Harveer Singh]

I see that there's a WIP patch for this and I peeked at it and looks good to me. Do you think we can land it or if you want me to commandeer it; I am okay with that too?

Comment 5

[Jan Varga [:janv]]

Let me look.

Comment 6

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #5)

Let me look.

Here’s the context for the pattern-based origin clearing work and the initial WIP patch:

1. The patch in its current form doesn’t make much sense on its own. It was mainly meant to give you some initial context and show where to start. As I mentioned back then, this really needs proper, dedicated testing before it’s meaningful.

2. You shouldn’t expect major performance improvements from the current patch. The real gains come only after we eliminate the expensive stat calls (see GetDirEntryKind).

3. The cached list is currently used only in two cases:

• when incremental origin initialization is enabled, and
• by the private-identity thumbnail optimization to speed up clearing during shutdown.
  Because of that, the existing test coverage for the cached list is quite incomplete.

4. Whether the patch stays under my name or you take it over and list me as the original author or co-author, that’s entirely up to you. (However, if you decide to take or copy the patch and land it without the necessary tests, please remove my name from it.)

5. Tests can live in TestQuotaManager.cpp, in xpcshell, or ideally both. Tests for clearing should cover:

• the state before temp storage initialization,
• the state when initialization fails,
• the state after successful initialization.

6. Bonus points:
   If you add a perf test, even just an xpcshell test with some simple time measurement, that would be great. If you look at what Florian has been experimenting with (perf metrics for xpcshell tests, asuth knows more), that would be even better.

Comment 7

[Harveer Singh]

Yes, I will see what we have for the coverage already and what gaps there might be. Since we are keeping the functional aspect of this feature same; I think existing functional regression tests might be sufficient but I will check and evaluate if there are any gaps that exists here. For the performance, it would be good to add an automated tests but at the very least I will be performing manual tests. We don't touch this section of code very often so manual tests should also be sufficient.

Actually I would expect your patch to already show decent performance gains because even though GetDirEntryKind might be expensive but we would no longer be iterating directories. I think we can avoid making a call to GetDirEntryKind altogether with your patch because the directories we get from QuotaManager cache must always be valid (a directory, exists on disk, etc.).

Comment 8

[Harveer Singh]

Attachment: Bug 1905068: Aiming to improve the performance of pattern basesd origin clearing.r=#dom-storage-reviewers

Comment 9

[Harveer Singh]

I have improved on your work and attached a patch to this bug. It's pref toggled, avoids directory traversal and avoids making unnecessary calls to filesystem. I haven't performed any performance testing on it yet; I will do that but theoretically it looks good.

Comment 10

[Harveer Singh]

Okay, I am getting close to finalizing the patch. Assigning it to myself.

Comment 11

[Harveer Singh]

Attachment: Bug 1905068: Only re-attempt clearing origin directories for transient errors.r=#dom-storage-reviewers

Comment 12

[Harveer Singh]

I tweaked some existing tests to get some performance numbers against new changes. I am not sure what would be a max number of origins a 95th or even 99th percentile profile would have but below are some of the results on my super-fast desktop machine running latest version of Ubuntu.

(Total origins under storage/default : total selected origins by pattern to be removed)

1. (3000:100)
   with my patch: 9ms
   without: 50ms

2. (30000:1000)
   with my patch: 73ms
   without: 400ms

To my surprise, directory traversal scaled pretty linearly and I expect these numbers to be very comparable across all major OSes.
My system has a very fast nvme, I expect the gap to be much wider on a slow rotating disk drive. I will see if I can get some help to test on that.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/29f957b6083f
https://hg.mozilla.org/integration/autoland/rev/bd7ac1e24ced
Aiming to improve the performance of pattern basesd origin clearing.r=dom-storage-reviewers,jari
https://github.com/mozilla-firefox/firefox/commit/a6790e888ba0
https://hg.mozilla.org/integration/autoland/rev/c20ce4906004
Only re-attempt clearing origin directories for transient errors.r=dom-storage-reviewers,jari

Comment 14

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/bd7ac1e24ced
https://hg.mozilla.org/mozilla-central/rev/c20ce4906004