Closed Bug 1531354 Opened 9 months ago Closed 5 months ago

Investigate if we can skip HashStore::Open during startup

Categories

(Toolkit :: Safe Browsing, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: dimi, Assigned: dimi)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [qf:p1:pageload])

Attachments

(6 files)

It seems we call HashStore::Open during startup(See Bug 1353956 Comment 15).

Theoretically, HashStore is used for update. This bug is to investigate why we call Hash::Open and see if we can skip it.

Whiteboard: [qf] → [qf:investigate]
Whiteboard: [qf:investigate] → [qf:p1:pageload]
Priority: -- → P2
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Priority: P2 → P1

List the scenarios that may trigger disk I/O during startup:

  1. Create test tables via SafeBrowsing update API which also leads to non-test tables file read[1][2]
  2. Scan the SafeBrowsing folder to get a list of v2 tables and then read corresponding .sbstore to ensure the correctness of those tables[3]
  3. Scheduled SafeBrowsing update
  4. Load SafeBrowsing prefixes

[1] https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/toolkit/components/url-classifier/SafeBrowsing.jsm#
[2] https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/toolkit/components/url-classifier/Classifier.cpp#343
[3] https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/toolkit/components/url-classifier/Classifier.cpp#846

See Also: → 779008
Depends on: 1540991
Depends on: 1540993

The work to be done here to get rid of HashStore::Open during startup is more complicated than I think initially.
I don't think I can finish it in 69, but I plan to fix it in 70

Depends on: 779008
Priority: P1 → P2
Priority: P2 → P1

The goal of the series of patches is to improve Safe Browsing performance by
skipping uncessary file IO.

The first two patches is to remove the dependency between LookupCache and HashStore, so HashStore is only
responsible for udpates.

Before this patch, LookupCacheV2 treats prefixes and completions
differently. It uses two data structures to maintain
prefixes:

  1. mPrefixSet to store prefixes from .pset
  2. mUpdateCompletions to store completions from .sbstore

After this patch

  1. LookupCacheV2 & LookupCacheV4 both use variable-length
    prefix set. mUpdateCompletions and mPrefixSet are removed and
    mVLPrefixSet is used to store all prefixes data.
  2. Move common function to base class.

Note that in this patch, conversion between 4/32 bytes prefixes and
mVLPrefixSet is not yet included, it will be handled in next patch.
This patch tries not to deal with any logic changes, only focus on refining
LookupCacheV2 & LookupCacheV4 class structure to use variable-length
prefixset for both classes.

  1. VariableLengthPrefixSet supports getting/setting prefixes with
    AddPrefixArray and AddCompletesArray

  2. VariableLengthPrefixSet supports passing prefix as an integer in
    Match API. This is because how V2 and V4 see prefixes as an integer
    works differently.

Completions are now stored in .vlpset, we can remove it from .sbstore
Functions related to optimize reading completions from .sbstore can also
be removed because it is no longer HashStore's responsibility

For Safe Browsing V2, Data for lookup(LookupCache) and data for update(HashStore)
are now separated. |RegenActiveTables| doesn't need to check the chunk
number in HashStore.

Create test entries via update introduces performance overhead.
We can store them directly in LookupCache and do not save test entries
to disk.

Test platform:
Aspire 3E 15 Intel Core i3-7100U (2.4 GHz, 3MB L3 cache) 
4 GB DDR4 Memory
1000 GB HDD

Before (not include fix in Bug 1353956, an update is triggered after startup):
https://perfht.ml/31q4Hbb

After (an update is triggered after startup):
https://perfht.ml/31ngBTv

Blocks: 1560630

This patch does the following:

  1. Remove testing files from disk because they are no longer required.
  2. Load completions from previous version of HashStore until an update
    is applied.
  3. Older version of HashStore(.sbstore) & PrefixSet(.vlpset) will be
    removed during an update

Nice improvement! (from the profiles)

Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ec7371e5a41
P1. Remove mPrefixSet and mUpdateCompletions from LookupCacheV2 and use mVLPresetSet. r=gcp
https://hg.mozilla.org/integration/autoland/rev/df0ff09bdcf9
P2. Use variable-length prefix set in LookupCacheV2. r=gcp
https://hg.mozilla.org/integration/autoland/rev/3799349af73f
P3. Do not store completion in HashStore. r=gcp
https://hg.mozilla.org/integration/autoland/rev/7417be59b320
P4. Skip reading hashstore in RegenActiveTables. r=gcp
https://hg.mozilla.org/integration/autoland/rev/cc7cce83765a
P5. Safe Browsing test entries are directly stored in LookupCache. r=gcp
https://hg.mozilla.org/integration/autoland/rev/93c33782f57d
P6. Remove unused testing files and load old version of prefixes data. r=gcp
Duplicate of this bug: 1540991
Duplicate of this bug: 1540993
Blocks: 1562875
Duplicate of this bug: 779008
Regressions: 1567076
Regressions: 1574450
Duplicate of this bug: 1553842
You need to log in before you can comment on or make changes to this bug.