Closed
Bug 1319286
Opened 7 years ago
Closed 7 years ago
Cache v4 table states in memory
Categories
(Toolkit :: Safe Browsing, defect, P2)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: hchang, Assigned: hchang)
References
Details
(Whiteboard: #sbv4-m3)
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
francois
:
review+
|
Details |
6.55 KB,
patch
|
Details | Diff | Splinter Review |
V4 hash completion requires table states. In order to make the completion more efficient, (completion request might be made very frequently compared to list update), we need a in-memory state cache.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → hchang
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: #sbv4-m3
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Since the table update and metadata retrieval would happen on the same thread, we are safe to have a intuitive lock-free in-memory cache in |nsIUrlClassiferDBService.getTables|. The only flaw of this approach is |nsIUrlClassiferDBService.getTables| will be blocked by table update. However, it is a existed issue since Bug 1305484 landed. Haven't made sure if it might cause any serious problem.
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8812995 [details] Bug 1319286 - Cache nsIUrlClassifierDBService.getTables result until next update. . https://reviewboard.mozilla.org/r/94512/#review94808 ::: toolkit/components/url-classifier/Classifier.h:163 (Diff revision 1) > nsTArray<nsCString> mActiveTablesCache; > uint32_t mHashKey; > // Stores the last time a given table was updated (seconds). > nsDataHashtable<nsCStringHashKey, int64_t> mTableFreshness; > + > + nsCString mMetadata; // In-memory metadata cache for v4 tables. I would suggest adding something like "(one table per line)" to that comment to hint at the format of that string and emphasize that it's not just for one table. ::: toolkit/components/url-classifier/Classifier.h:164 (Diff revision 1) > uint32_t mHashKey; > // Stores the last time a given table was updated (seconds). > nsDataHashtable<nsCStringHashKey, int64_t> mTableFreshness; > + > + nsCString mMetadata; // In-memory metadata cache for v4 tables. > + bool mRequiresMetadataReload; // Whether the metacache needs to be reloaded. suggestion: "Whether the metadata needs to be reloaded from disk." ::: toolkit/components/url-classifier/Classifier.cpp:391 (Diff revision 1) > } > > aResult.Append('\n'); > } > > + // WriteMetadata and LoadMetadata is guaranteed to be called "are guaranteed" ::: toolkit/components/url-classifier/Classifier.cpp:400 (Diff revision 1) > - // Specifically for v4 tables. > + // Specifically for v4 tables. > - nsCString metadata; > - nsresult rv = LoadMetadata(mRootStoreDirectory, metadata); > - NS_ENSURE_SUCCESS_VOID(rv); > - aResult.Append(metadata); > + nsresult rv = LoadMetadata(mRootStoreDirectory, mMetadata); > + if (NS_SUCCEEDED(rv)) { > + mRequiresMetadataReload = false; > + } else { > + LOG(("Failed to reload metadata.")); Should we make this a warning or do we expect this to happen normally?
Attachment #8812995 -
Flags: review?(francois) → review+
Comment 4•7 years ago
|
||
(In reply to Henry Chang [:henry][:hchang] from comment #2) > Since the table update and metadata retrieval would happen on the same > thread, we are safe to have a intuitive lock-free in-memory cache in > |nsIUrlClassiferDBService.getTables|. The only flaw of this > approach is |nsIUrlClassiferDBService.getTables| will be blocked by table > update. However, it is > a existed issue since Bug 1305484 landed. Haven't made sure if it might > cause any serious problem. Given the potential impact of this, I would suggest we file a follow-up bug to investigate whether we can work-around this lock or recover if we get blocked, to make sure we don't get another bug 1315097.
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to François Marier [:francois] from comment #4) > (In reply to Henry Chang [:henry][:hchang] from comment #2) > > Since the table update and metadata retrieval would happen on the same > > thread, we are safe to have a intuitive lock-free in-memory cache in > > |nsIUrlClassiferDBService.getTables|. The only flaw of this > > approach is |nsIUrlClassiferDBService.getTables| will be blocked by table > > update. However, it is > > a existed issue since Bug 1305484 landed. Haven't made sure if it might > > cause any serious problem. > > Given the potential impact of this, I would suggest we file a follow-up bug > to investigate whether we can work-around this lock or recover if we get > blocked, to make sure we don't get another bug 1315097. Just realize Classifier::TableRequest has been a function which would be blocked by disk I/O [1] since very beginning so Bug 1315097 didn't make it worse. Besides, |nsIURIClassifier.classify| is inevitably blocked by the worker thread [2], which means we can do nothing but wait during table update. The implication is |nsIUrlClassiferDBService.getTables| is supposed to be called back with the updated table states if the API is called in the middle of table update. [1] https://dxr.mozilla.org/mozilla-central/rev/0534254e9a40b4bade2577c631fe4cfa0b5db41d/toolkit/components/url-classifier/Classifier.cpp#357 [2] https://dxr.mozilla.org/mozilla-central/rev/0534254e9a40b4bade2577c631fe4cfa0b5db41d/toolkit/components/url-classifier/nsUrlClassifierProxies.cpp#142
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
According to comment 5, the entire |Classifier::TableRequest| function involves disk I/O no matter v2 or v4, which means all results need to be cached. Also, I changed the timing when we mark the cached result outdated to prevent from non-atomic table updates. Regarding the test case, I verify the caching mechanism in test_listmanager (again!) by calling two nsIUrlClassifierDBService.getTables in a row and compare if the results are the same. This is meaningful because the first call is guaranteed the one right after the database is just updated. Francois, would you like to have a review again? Thanks!
Flags: needinfo?(francois)
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8812995 [details] Bug 1319286 - Cache nsIUrlClassifierDBService.getTables result until next update. . https://reviewboard.mozilla.org/r/94512/#review95408 r+
Updated•7 years ago
|
Flags: needinfo?(francois)
Pushed by hchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b3574d03261b Cache nsIUrlClassifierDBService.getTables result until next update. r=francois.
Comment 10•7 years ago
|
||
Sorry had to backout this for GTest Assertion failures, i.e., https://treeherder.mozilla.org/logviewer.html#?job_id=7153256&repo=autoland#L13765
Flags: needinfo?(hchang)
Comment 11•7 years ago
|
||
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a35328bd4d07 Backed out changeset b3574d03261b for GTest Assertion failures
Assignee | ||
Comment 12•7 years ago
|
||
Oops I forgot to add gtest for my previous push try :( Sorry for that.
Flags: needinfo?(hchang)
Assignee | ||
Comment 13•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=774d823ca816
Assignee | ||
Comment 14•7 years ago
|
||
Waiting for try run result for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=774d823ca8160c3af849c12b2c95eb3efdd121ab
Comment 15•7 years ago
|
||
Pushed by hchang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8d64aeb611 Cache nsIUrlClassifierDBService.getTables result until next update. r=francois.
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4f8d64aeb611
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•