Closed Bug 1319286 Opened 3 years ago Closed 3 years ago

Cache v4 table states in memory

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: hchang, Assigned: hchang)

References

Details

(Whiteboard: #sbv4-m3)

Attachments

(2 files)

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.
Blocks: 1276826
Assignee: nobody → hchang
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: #sbv4-m3
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 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+
(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.
(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
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 on attachment 8812995 [details]
Bug 1319286 - Cache nsIUrlClassifierDBService.getTables result until next update. .

https://reviewboard.mozilla.org/r/94512/#review95408

r+
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.
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)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a35328bd4d07
Backed out changeset b3574d03261b for GTest Assertion failures
Oops I forgot to add gtest for my previous push try :( Sorry for that.
Flags: needinfo?(hchang)
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8d64aeb611
Cache nsIUrlClassifierDBService.getTables result until next update. r=francois.
https://hg.mozilla.org/mozilla-central/rev/4f8d64aeb611
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.