Closed Bug 1353853 Opened 7 years ago Closed 7 years ago

nsUrlClassifierDBService::BuildTables should use cached preferences

Categories

(Toolkit :: Safe Browsing, enhancement, P1)

Product:
Toolkit
Component:
Safe Browsing
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kanru, Assigned: tnguyen)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1353853 - Cache preferences when doing channel classify
59 bytes, text/x-review-board-request
francois
: review+
Details 
This is found in bug 1352525

Sort by times read in 5 minutes
 2516   urlclassifier.trackingWhitelistTable: test-trackwhite-simple,mozstd-trackwhite-digest256
 1867   urlclassifier.trackingTable: test-track-simple,base-track-digest256
 1867   urlclassifier.phishTable: goog-phish-shavar,goog-phish-proto,test-phish-simple
 1867   urlclassifier.blockedTable: test-block-simple,mozplugin-block-digest256
 1867   urlclassifier.malwareTable: goog-malware-shavar,goog-unwanted-shavar,goog-malware-proto,goog-unwanted-proto,test-malware-simple,test-unwanted-simple
 1865   urlclassifier.skipHostnames: null

Sort by bytes read
    224040  urlclassifier.malwareTable
    125800  urlclassifier.trackingWhitelistTable
     97084  urlclassifier.phishTable
     80281  urlclassifier.blockedTable
     70946  urlclassifier.trackingTable

Reading from the Preferences is not fast and this function is called every time when there is a channel connect. It doesn't make sense to rebuild the table repeatedly when the content doesn't change.

It looks like nsUrlClassifierDBService::ReadTablesFromPrefs() has already done some caching. We can probably reuse that.
Henry, any chance you could take a look at this one please?
Flags: needinfo?(hchang)
Some of the prefs are also used outside nsUrlClassifierDBService but it shouldn't be a big deal 
if we make the cached prefs static.
Flags: needinfo?(hchang)
Assignee: nobody → hchang
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee: hchang → tnguyen
MozReview-Commit-ID: 8XRguvmbM44
Comment on attachment 8855261 [details] [diff] [review]
Cache tables preferences map -WIP

Review of attachment 8855261 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1643,5 @@
>  nsUrlClassifierDBService::BuildTables(bool aTrackingProtectionEnabled,
>                                        nsCString &tables)
>  {
> +  BuildTablesFromMap(MALWARE_TABLE_PREF, tables);
> +  BuildTablesFromMap(PHISH_TABLE_PREF, tables);

Can we also cache the table results? It looks like it's creating the same output string again and again.
Comment on attachment 8855261 [details] [diff] [review]
Cache tables preferences map -WIP

Oh, sure, we could do that. Thanks for your suggestion.
I should change detail of the patch to WIP, because likely we still have to cache preference list here 
https://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/netwerk/base/nsChannelClassifier.cpp#370
and 
https://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/netwerk/base/nsChannelClassifier.cpp#737
Attachment #8855261 - Attachment description: Cache tables preferences map → Cache tables preferences map -WIP
Comment on attachment 8855261 [details] [diff] [review]
Cache tables preferences map -WIP

Review of attachment 8855261 [details] [diff] [review]:
-----------------------------------------------------------------

As far as I can recall, there are still few other places where we read prefs per-channel.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1479,5 @@
>  }
>  
>  nsresult
>  nsUrlClassifierDBService::ReadTablesFromPrefs()
>  {

I also think this function doesn't make much difference because it's only called in Init() and Observe().

@@ +1643,5 @@
>  nsUrlClassifierDBService::BuildTables(bool aTrackingProtectionEnabled,
>                                        nsCString &tables)
>  {
> +  BuildTablesFromMap(MALWARE_TABLE_PREF, tables);
> +  BuildTablesFromMap(PHISH_TABLE_PREF, tables);

This approach seems to tightly couple BuildTables() and ReadTablesFromPrefs(). ReadTablesFromPrefs() has to build a map which includes all necessary info for BuildTables(). (unless we add some code to ensure that condition is guaranteed satisfied.)

I think just caching the last build result and invalidate the cached result whenever needed. (e.g. clear the cached result in Observe())

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
@@ +494,5 @@
> +    if (StringEndsWith(data, NS_LITERAL_STRING("lastupdatetime")) ||
> +        StringEndsWith(data, NS_LITERAL_STRING("nextupdatetime"))) {
> +      return NS_OK;
> +    }
> +

I don't think this makes much difference cuz we are in Observe().
(In reply to Henry Chang [:henry][:hchang] from comment #6)
> Comment on attachment 8855261 [details] [diff] [review]
> Cache tables preferences map -WIP
> 
> Review of attachment 8855261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As far as I can recall, there are still few other places where we read prefs
> per-channel.
> 
> ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
> @@ +1479,5 @@
> >  }
> >  
> >  nsresult
> >  nsUrlClassifierDBService::ReadTablesFromPrefs()
> >  {
> 
> I also think this function doesn't make much difference because it's only
> called in Init() and Observe().
> 
> @@ +1643,5 @@
> >  nsUrlClassifierDBService::BuildTables(bool aTrackingProtectionEnabled,
> >                                        nsCString &tables)
> >  {
> > +  BuildTablesFromMap(MALWARE_TABLE_PREF, tables);
> > +  BuildTablesFromMap(PHISH_TABLE_PREF, tables);
> 
> This approach seems to tightly couple BuildTables() and
> ReadTablesFromPrefs(). ReadTablesFromPrefs() has to build a map which
> includes all necessary info for BuildTables(). (unless we add some code to
> ensure that condition is guaranteed satisfied.)
> 

For example, 

We can check if |kAllPrefsTables| contains |aData| to avoid potential inconsistency.

http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#2177
Thanks Henry

(In reply to Henry Chang [:henry][:hchang] from comment #6)
> Comment on attachment 8855261 [details] [diff] [review]
> Cache tables preferences map -WIP
> 
> Review of attachment 8855261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As far as I can recall, there are still few other places where we read prefs
> per-channel.
> 
> ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
> @@ +1479,5 @@
> >  }
> >  
> >  nsresult
> >  nsUrlClassifierDBService::ReadTablesFromPrefs()
> >  {
> 
> I also think this function doesn't make much difference because it's only
> called in Init() and Observe()

> ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
> @@ +494,5 @@
> > +    if (StringEndsWith(data, NS_LITERAL_STRING("lastupdatetime")) ||
> > +        StringEndsWith(data, NS_LITERAL_STRING("nextupdatetime"))) {
> > +      return NS_OK;
> > +    }
> > +
> 
> I don't think this makes much difference cuz we are in Observe().

Yep, it may not make much difference, but re-reading tables in lastupdatetime, nextupdatetime also is redundant. It would be better to remove them here or we can create a nit bug to do that 

> > +  BuildTablesFromMap(PHISH_TABLE_PREF, tables);
> 
> This approach seems to tightly couple BuildTables() and
> ReadTablesFromPrefs(). ReadTablesFromPrefs() has to build a map which
> includes all necessary info for BuildTables(). (unless we add some code to
> ensure that condition is guaranteed satisfied.)
> 
> I think just caching the last build result and invalidate the cached result
> whenever needed. (e.g. clear the cached result in Observe())
> 
We may not want to cache the last result which may depend on Tracking Protection feature on/off (unless we have to observe TP preference). Another approach is just caching the tables which are necessary for BuildTables and update them in Observe(). 
The approach here is caching all tables from prefs then people can use them later, such as buildtables or maybe getting whitelist tables from nsChannelClassifier (perhaps I should do it in Utils class).

> 
> We can check if |kAllPrefsTables| contains |aData| to avoid potential
> inconsistency.
> 
> http://searchfox.org/mozilla-central/rev/
> b8cce081200129a1307e2a682f121135b5ca1d19/toolkit/components/url-classifier/
> nsUrlClassifierDBService.cpp#2177

Thanks Henry for your great finding, 
Sure, and also need to check 
http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1637

I will update this tomorrow.
Attachment #8855698 - Flags: review?(hchang)
Attachment #8855698 - Flags: review?(francois)
Comment on attachment 8855261 [details] [diff] [review]
Cache tables preferences map -WIP

Hi Henry, could you please take a look at the patches?
Thanks
Attachment #8855261 - Attachment is obsolete: true
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review130274

Generally looks good. Using nsTArray<nsCString> to store preferences and redesigning BuildTableFromMap will make it the best!

::: toolkit/components/url-classifier/Classifier.h:26
(Diff revision 1)
>  
>  /**
>   * Maintains the stores and LookupCaches for the url classifier.
>   */
>  class Classifier {
>  public:

Is this a accidental deletion?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:145
(Diff revision 1)
>  // thread.
>  static bool gShuttingDownThread = false;
>  
>  static mozilla::Atomic<int32_t> gFreshnessGuarantee(CONFIRM_AGE_DEFAULT_SEC);
>  
> +static const char* kAllPrefsTables[] = {

We can use nsTArray<nsCString> = { ... } and take advantages of their low level string/array maniputations (e.g. nsTArray::Contains, nsACString.Equals)

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1651
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
>  void
> -nsUrlClassifierDBService::BuildTables(bool aTrackingProtectionEnabled,
> +nsUrlClassifierDBService::BuildTablesFromMap(const PrefsTablesMap& aMap,

The function doesn't suggest its behavior very well. It ends up with "appending" something to the input as the output. We should change its name or make it take a list of string (e.g. aPrefKeys)

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
(Diff revision 1)
>  
>    // If we don't know about this table at all, or are disallowing completions
>    // for it, skip completion checks.
>    if (!mGethashTables.Contains(tableName) ||
>        mDisallowCompletionsTables.Contains(tableName)) {
> -    return false;

Is this line accidentally removed?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:2156
(Diff revision 1)
>      } else if (NS_LITERAL_STRING(CONFIRM_AGE_PREF).Equals(aData)) {
>        gFreshnessGuarantee = Preferences::GetInt(CONFIRM_AGE_PREF,
>          CONFIRM_AGE_DEFAULT_SEC);
> +    } else {
> +      // Check if we need to read tables again
> +      for (uint8_t i = 0; i < ArrayLength(kAllPrefsTables); i++) {

We can use nsTArray::Contains instead of searching on our own.
Attachment #8855697 - Flags: review?(hchang)
Comment on attachment 8855698 [details]
Bug 1353853 - P2 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127578/#review130298

The current pref callback register/unregister may not be paired. One solution is to obeserve all necessary prefs changes in any singleton object (e.g. nsURLClassifierDBService) and take from that pref store. Of cource we can also create a private pref store for nsChannelClassifer specifically. It's up to you :)

::: netwerk/base/nsChannelClassifier.cpp:939
(Diff revision 1)
>        mChannel->Cancel(NS_ERROR_ABORT);
>        mChannel->Resume();
>      }
>  
>      RemoveShutdownObserver();
> +    for (size_t i = 0; i < ArrayLength(kUrlClassifierPrefs); i++) {

This place seems to be used to remove "per-channel" observer/callback. Your callback is registered only once in the first ever nsChannelClassifier ctor. Removing inexisted callback might not be an issue but there will be memory leak issue if no nsChannelClassifier received profile-change-net-teardown during shutdown.
Attachment #8855698 - Flags: review?(hchang)
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review130296

::: toolkit/components/url-classifier/Classifier.h:26
(Diff revision 1)
>  
>  /**
>   * Maintains the stores and LookupCaches for the url classifier.
>   */
>  class Classifier {
>  public:

classifier ProviderDictType is not used any where (ProviderDictType type exits in nsUrlClassifierUtils.h)
We could remove it or am I missing?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
(Diff revision 1)
>  
>    // If we don't know about this table at all, or are disallowing completions
>    // for it, skip completion checks.
>    if (!mGethashTables.Contains(tableName) ||
>        mDisallowCompletionsTables.Contains(tableName)) {
> -    return false;

Oops, thanks for finding this
> 
> This place seems to be used to remove "per-channel" observer/callback. Your
> callback is registered only once in the first ever nsChannelClassifier ctor.
> Removing inexisted callback might not be an issue but there will be memory
> leak issue if no nsChannelClassifier received profile-change-net-teardown
> during shutdown.

Thanks Henry for your review :)
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review130530

::: toolkit/components/url-classifier/nsUrlClassifierDBService.h:93
(Diff revision 1)
>    static nsIThread* BackgroundThread();
>  
>    static bool ShutdownHasStarted();
>  
>  private:
> +  typedef nsClassHashtable<nsCharPtrHashKey, nsCString> PrefsTablesMap;

There needs to be a comment explaining that this maps a preference name to its value (the list of tables it contains.

Also, `nsCharPtrHashKey` doesn't make sense here since there are no hashes here.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.h:148
(Diff revision 1)
>    nsTArray<nsCString> mGethashTables;
>  
>    // The list of tables that should never be hash completed.
>    nsTArray<nsCString> mDisallowCompletionsTables;
>  
> +  // Cache tables then we could do lookup

I would replace this comment with:

    // Comma-separated list of tables to use in lookups.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.h:149
(Diff revision 1)
>  
>    // The list of tables that should never be hash completed.
>    nsTArray<nsCString> mDisallowCompletionsTables;
>  
> +  // Cache tables then we could do lookup
> +  nsCString mLookupableTables;

`Lookupable` is not really a word. Maybe we should use `mBaseTables` and `mTrackingProtectionTables`.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:67
(Diff revision 1)
>  {
>    if (tables.IsEmpty()) {
>      return NS_OK;
>    }
>  
> -  // We don't check mCheckMalware and friends because BuildTables never
> +  // We don't check mCheckMalware and friends because disabled table is

"because disabled tables are never included."

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1487
(Diff revision 1)
>  {
> -  nsCString allTables;
> +  PrefsTablesMap prefsTablesMap;
> +  // Read tables from Prefs to map
> +  for (uint8_t i = 0; i < ArrayLength(kAllPrefsTables); i++) {
> -  nsCString tables;
> +    nsCString tables;
> -  Preferences::GetCString(PHISH_TABLE_PREF, &allTables);
> +    Preferences::GetCString(kAllPrefsTables[i], &tables);

To make this easier to read, we could create an `nsCString& prefName = kAllPrefsTables[i]` and use that in the loop.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1651
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
>  void
> -nsUrlClassifierDBService::BuildTables(bool aTrackingProtectionEnabled,
> +nsUrlClassifierDBService::BuildTablesFromMap(const PrefsTablesMap& aMap,

I agree with Henry, I would suggest `AppendPrefTables()`.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1721
(Diff revision 1)
>    RefPtr<nsUrlClassifierClassifyCallback> callback =
>      new nsUrlClassifierClassifyCallback(c);
>  
>    if (!callback) return NS_ERROR_OUT_OF_MEMORY;
>  
> -  nsAutoCString tables;
> +  nsCString tables = aTrackingProtectionEnabled ?

Using the `mBaseTables` and `mTrackingProtectionTables` I suggested above, this would become:

```
nsCString tables = mBaseTables
if (aTrackingProtectionEnabled) {
   tables.Append(mTrackingProtectionTables)
}

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:2215
(Diff revision 1)
>  
>    mCompleters.Clear();
>  
>    nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
>    if (prefs) {
> -    prefs->RemoveObserver(CHECK_MALWARE_PREF, this);
> +    for (uint8_t i = 0; i < ArrayLength(kAllPrefsTables); i++) {

These prefs are not included in `kAllPrefsTables` and their observer needs to be removed manually here:

- `CHECK_MALWARE_PREF`
- `CHECK_PHISHING_PREF`
- `CHECK_BLOCKED_PREF`
- `CONFIRM_AGE_PREF`
- `GETHASH_NOISE_PREF`

It looks like that last one was missing in the original code, but we do add an observer for it in `nsUrlClassifierDBService::Init()`.
Attachment #8855697 - Flags: review?(francois) → review-
Comment on attachment 8855698 [details]
Bug 1353853 - P2 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127578/#review130538

::: netwerk/base/nsChannelClassifier.cpp:60
(Diff revision 1)
>  // protection list should be lowered.
>  static bool sLowerNetworkPriority = false;
>  static bool sIsInited = false;
> +static bool sAllowListExample = false;
> +static nsCString sTrackingWhitelist("");
> +static nsCString sSkipHostname("");

`sSkipHostnames`

::: netwerk/base/nsChannelClassifier.cpp:66
(Diff revision 1)
>  
>  #undef LOG
>  #define LOG(args)     MOZ_LOG(gChannelClassifierLog, LogLevel::Debug, args)
>  #define LOG_ENABLED() MOZ_LOG_TEST(gChannelClassifierLog, LogLevel::Debug)
>  
> +#define URLCLASSIFIER_SKIP_HOSTNAME       "urlclassifier.skipHostnames"

`URLCLASSIFIER_SKIP_HOSTNAMES` (plural)

::: netwerk/base/nsChannelClassifier.cpp:67
(Diff revision 1)
>  #undef LOG
>  #define LOG(args)     MOZ_LOG(gChannelClassifierLog, LogLevel::Debug, args)
>  #define LOG_ENABLED() MOZ_LOG_TEST(gChannelClassifierLog, LogLevel::Debug)
>  
> +#define URLCLASSIFIER_SKIP_HOSTNAME       "urlclassifier.skipHostnames"
> +#define URLCLASSIFIER_TRACKINGWHITELIST   "urlclassifier.trackingWhitelistTable"

`URLCLASSIFIER_TRACKING_WHITELIST` for consistency

::: netwerk/base/nsChannelClassifier.cpp:75
(Diff revision 1)
> +  URLCLASSIFIER_SKIP_HOSTNAME,
> +  URLCLASSIFIER_TRACKINGWHITELIST
> +};
> +
> +static void
> +UCPrefsChangedCallback(const char* aPref, void* aClosure)

Are you supposed to do something with `aClosure`?
Attachment #8855698 - Flags: review?(francois) → review-
Also, we can probably merge these two patches together. They are essentially doing the same thing and they're both quite small.
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review130722

::: toolkit/components/url-classifier/nsUrlClassifierDBService.h:93
(Diff revision 1)
>    static nsIThread* BackgroundThread();
>  
>    static bool ShutdownHasStarted();
>  
>  private:
> +  typedef nsClassHashtable<nsCharPtrHashKey, nsCString> PrefsTablesMap;

Thanks Francois, added comment.
but I am not quite sure your comment, nsCharPtrHashKey should be a keytype of hash table which is defined in http://searchfox.org/mozilla-central/rev/eace920a0372051a11d8d275bd9b8f14f3024ecd/xpcom/ds/nsHashKeys.h#484
Changed to use nsCStringHashkey anyway
Attachment #8855698 - Attachment is obsolete: true
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #21)
> Comment on attachment 8855697 [details]
> Bug 1353853 - Cache preferences when doing channel classify
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/127576/diff/2-3/

Rebased commit and updated commit message
Whiteboard: [qf] → [qf:p1]
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review131814

::: netwerk/base/nsChannelClassifier.cpp:70
(Diff revision 4)
> +/**
> + * It is not recommended to read from Preference everytime a channel is
> + * connected.
> + * That is not fast and we should cache preference values and reuse them
> + */
> +class CachedPrefs final

Wrap with unnamed namespace to avoid naming pollution.

::: netwerk/base/nsChannelClassifier.cpp:117
(Diff revision 4)
> +// protection list should be lowered.
> +bool CachedPrefs::sLowerNetworkPriority = false;
> +bool CachedPrefs::sAllowListExample = false;
> +nsCString CachedPrefs::sTrackingWhitelist = EmptyCString();
> +nsCString CachedPrefs::sSkipHostnames = EmptyCString();
> +StaticAutoPtr<CachedPrefs> CachedPrefs::sInstance;

Are you assured this object (CachedPref) will be destructed before the Preference system is still alive? I haven't had a closer look to ClearOnShutdown. Maybe you know the answer already. Just need to confirm here.
Attachment #8855697 - Flags: review?(hchang)
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review131820

I didn't look through the entire patch but it seems very close the what I imagined in the first place. Good job!
Could you help confirm the necessity of UnregisterCallback()? IMO, we don't need to call that function so
that we can simplify the singleton pattern you are using right now. (Actually the singleton pattern you
are current using is not that correct. Only GetInstance() needs to be static.)

::: netwerk/base/nsChannelClassifier.cpp:76
(Diff revision 4)
> +{
> +public:
> +  static void Init();
> +  static CachedPrefs* GetInstance();
> +
> +  static bool IsAllowListExample() { return sAllowListExample;}

Only "GetInstance()" needs to be static if you are using the singleton pattern.

::: netwerk/base/nsChannelClassifier.cpp:79
(Diff revision 4)
> +  static CachedPrefs* GetInstance();
> +
> +  static bool IsAllowListExample() { return sAllowListExample;}
> +  static bool IsLowerNetworkPriority() { return sLowerNetworkPriority;}
> +  static bool IsAnnotateChannelEnabled() { return sAnnotateChannelEnabled;}
> +  static void GetTrackingWhiteList(nsACString& aWhiteList)

I would prefer just return the result like "IsLowerNetworkPriority"

::: netwerk/base/nsChannelClassifier.cpp:167
(Diff revision 4)
> +}
> +CachedPrefs::~CachedPrefs()
> +{
> +  MOZ_COUNT_DTOR(CachedPrefs);
> +  for (size_t i = 0; i < ArrayLength(kUrlClassifierPrefs); i++) {
> +    Preferences::UnregisterCallback(CachedPrefs::OnPrefsChange, kUrlClassifierPrefs[i]);

I wonder if we really need to call Unregister().

http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/modules/libpref/Preferences.cpp#1779

It looks the main purpose of unregister is to remove the closure that we register with. If Unregister() is  not required, we can simply use "Meyer's Singleton" to simplify pretty much everything.
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review131820

Checking the implementation of Preferences::RegisterCallback, I think "UnregisterCallback" is needed regardless of the value of closure we registered. Sorry I was wrong :(

So, the rest of issue we need to confirm is whether CachedPref will be destructed in a proper time.
(In reply to Henry Chang [:henry][:hchang] from comment #26)
> Comment on attachment 8855697 [details]
> Bug 1353853 - Cache preferences when doing channel classify
> 
> https://reviewboard.mozilla.org/r/127576/#review131820
> 
> Checking the implementation of Preferences::RegisterCallback, I think
> "UnregisterCallback" is needed regardless of the value of closure we
> registered. Sorry I was wrong :(
> 
> So, the rest of issue we need to confirm is whether CachedPref will be
> destructed in a proper time.

Thanks Henry,
It should be ok, afaik, releasing service/component should be the last phase of shutdown. 
So Preferences should be available when we are trying to free smart ptr in ClearOnShutdown
> So Preferences should be available when we are trying to free smart ptr in
> ClearOnShutdown

Just double checked [1] : clear smart pointer on shutdown (included StaticAutoPtr) and [2] : release service including preferences
[1] https://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/xpcom/build/XPCOMInit.cpp#939
[2] https://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/xpcom/build/XPCOMInit.cpp#952
Thanks Thomas!

I don't find any issue more. We should be good to ask for Francois's review now :)
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

cancel request due to failures on debug build Try
Attachment #8855697 - Flags: review?(hchang)
Attachment #8855697 - Flags: review?(francois)
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #19)
> Comment on attachment 8855697 [details]
> Bug 1353853 - Cache preferences when doing channel classify
> 
> https://reviewboard.mozilla.org/r/127576/#review130722
> 
> ::: toolkit/components/url-classifier/nsUrlClassifierDBService.h:93
> (Diff revision 1)
> >    static nsIThread* BackgroundThread();
> >  
> >    static bool ShutdownHasStarted();
> >  
> >  private:
> > +  typedef nsClassHashtable<nsCharPtrHashKey, nsCString> PrefsTablesMap;
> 
> Thanks Francois, added comment.
> but I am not quite sure your comment, nsCharPtrHashKey should be a keytype
> of hash table which is defined in
> http://searchfox.org/mozilla-central/rev/
> eace920a0372051a11d8d275bd9b8f14f3024ecd/xpcom/ds/nsHashKeys.h#484
> Changed to use nsCStringHashkey anyway

Ignore my comment. I thought that line was referring to Safe Browsing hashes, but it's actually referring to the keys in that data structure (hash table).

Sorry for the confusion!
Hmm, it seems that using static nsCString and global static nsTArray<> is not a good idea which may create global static destructors and constructors. I did not know about that :(
That's why I could see in the try failure, there's always 8 bytes leak from static nsCString
I have to change back to use static POD type and nsCString *
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #38)
> Comment on attachment 8855697 [details]
> Bug 1353853 - Cache preferences when doing channel classify
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/127576/diff/6-7/

This is only rebasing commit
Hi Francois,
I discussed with Henry, and it seems the old patch using a map is not the best idea in term of performance. We should cache all preferences string to class variables instead.
It would be a little bit ugly if we have too many preference variables (8 strings in nsUrlClassifierDBService, and 4 bools). So I would prefer store them in a struct array indexed by enum id (I created a macro to facilitate array and enum index pairing)
Could you please take a look?
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #42)
> Hi Francois,
> I discussed with Henry, and it seems the old patch using a map is not the
> best idea in term of performance. We should cache all preferences string to
> class variables instead.
> It would be a little bit ugly if we have too many preference variables (8
> strings in nsUrlClassifierDBService, and 4 bools). So I would prefer store
> them in a struct array indexed by enum id (I created a macro to facilitate
> array and enum index pairing)
> Could you please take a look?

I can explain this a little bit more (since that was me who pointed this out)

IMO, the point of this bug [2] is to get rid of the frequent hash map lookup 
which occurs in the preferences system internal [1]. Even though its hash 
implementation has constant lookup time, it's still required calculating 
the hash key from the string, which is proportional to the string length. 
I cannot tell if calculating the string hash key is costly but that
is the one and the only one point to this bug [2]. 

In the previous implementation, a hash map is used to store cached 
preferences, which is equivalent to looking up the preferences system's
internal hash map in terms of average time complexity. So, I suggested
Thomas to just use individual variables to cache preferences instead a
hash map.


[1] http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/modules/libpref/prefapi.cpp#483 
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1352525#c0
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review133516

I think that overall I preferred your first approach. I can see that the prefs are listed in a few places and the duplication is annoying, but this new enum+macros construction seems to make things a lot harder to read.

::: netwerk/base/nsChannelClassifier.cpp:73
(Diff revision 8)
> +/**
> + * It is not recommended to read from Preference everytime a channel is
> + * connected.
> + * That is not fast and we should cache preference values and reuse them
> + */
> +class CachedPrefs final

In this file, I think it might be overkill to create a complicated caching system for the prefs.

Given we only have 5 prefs to look at, I much preferred the previous approach which used a handful of static variables.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:70
(Diff revision 8)
>    if (tables.IsEmpty()) {
>      return NS_OK;
>    }
>  
> -  // We don't check mCheckMalware and friends because BuildTables never
> -  // includes a table that is not enabled.
> +  // We don't check GetMalwareEnabled() and friends because disabled tables are
> +  // nerver included

"never"

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
(Diff revision 8)
>    // The application is about to quit
>    observerService->AddObserver(this, "quit-application", false);
>    observerService->AddObserver(this, "profile-before-change", false);
>  
> -  // XXX: Do we *really* need to be able to change all of these at runtime?
> +  Preferences::AddAtomicUintVarCache(&gFreshnessGuarantee, CONFIRM_AGE_PREF);
> -  // Note: These observers should only be added when everything else above has

We should make sure we maintain these comments until we've addressed them.
Attachment #8855697 - Flags: review?(francois) → review-
Changed to the first approach.
See Also: → 1357898
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review134562

The dbservice parts look good to me. Thanks for simplifying this Thomas!

For the channel classifier, I'm wondering whether we could simplify this further by not introducing a new class.

We could put the observers in nsChannelClassifier's constructor and destructor, then just have static variables for the three new prefs (so five static variables in total).

::: netwerk/base/nsChannelClassifier.cpp:139
(Diff revision 9)
> +                               "privacy.trackingprotection.annotate_channels");
> +  Preferences::AddBoolVarCache(&sLowerNetworkPriority,
> +                               "privacy.trackingprotection.lower_network_priority");
> +  Preferences::AddBoolVarCache(&sAllowListExample,
> +                               "channelclassifier.allowlist_example");
> +  for (size_t i = 0; i < ArrayLength(kUrlClassifierPrefs); i++) {

I think that using a loop and an array for just 2 prefs is a little bit overkill.

We could simply have two `Preferences::RegisterCallbackAndCall()` calls here.

::: netwerk/base/nsChannelClassifier.cpp:166
(Diff revision 9)
> +
> +CachedPrefs::~CachedPrefs()
> +{
> +  MOZ_COUNT_DTOR(CachedPrefs);
> +
> +  for (size_t i = 0; i < ArrayLength(kUrlClassifierPrefs); i++) {

Similarly, we can simply call that function twice for each of the prefs in the array.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.h:51
(Diff revision 9)
> +#define CHECK_MALWARE_DEFAULT   false
> +
> +#define CHECK_PHISHING_PREF     "browser.safebrowsing.phishing.enabled"
> +#define CHECK_PHISHING_DEFAULT  false
> +
> +#define CHECK_BLOCKED_PREF    "browser.safebrowsing.blockedURIs.enabled"

nit: we may as well align the values with the ones above

::: toolkit/components/url-classifier/nsUrlClassifierDBService.h:146
(Diff revision 9)
>  
>    // Check if the key is on a known-clean host.
>    nsresult CheckClean(const nsACString &lookupKey,
>                        bool *clean);
>  
> -  // Read everything into mGethashTables and mDisallowCompletionTables
> +  // Cache table from prefs

nit: I think this comment is unnecessary.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:125
(Diff revision 9)
>  
>  // Once we've committed to shutting down, don't do work in the background
>  // thread.
>  static bool gShuttingDownThread = false;
>  
> -static mozilla::Atomic<int32_t> gFreshnessGuarantee(CONFIRM_AGE_DEFAULT_SEC);
> +static mozilla::Atomic<uint32_t, Relaxed> gFreshnessGuarantee(CONFIRM_AGE_DEFAULT_SEC);

I filed bug 1357898 to get rid of this pref entirely.
Attachment #8855697 - Flags: review?(francois) → review-
Depends on: 1357898
See Also: 1357898 → 
(In reply to François Marier [:francois] from comment #48)
> Comment on attachment 8855697 [details]
> Bug 1353853 - Cache preferences when doing channel classify
> 
> https://reviewboard.mozilla.org/r/127576/#review134562
> 
> The dbservice parts look good to me. Thanks for simplifying this Thomas!
> 
> For the channel classifier, I'm wondering whether we could simplify this
> further by not introducing a new class.
> 
> We could put the observers in nsChannelClassifier's constructor and
> destructor, then just have static variables for the three new prefs (so five
> static variables in total).

Thanks Francois.
I don't think we should put the observers in nsChannelClassifier's ctor and remove them in dtor. Because we still have to read the preference string each time we construct nsChannelClassifier (Channel BeginConnect for example), and the problem remains the same.
Another way I tried , we use new static variables to store preferences(use RegisterCallbackAndCall) and register 1 time for the very first init of all nsChannelClassifier (though checking sInited static bool, like we did before if (!sIsInited) { sIsInited = true; ...} ), we still have to UnregisterCallback when FF shutdown. Calling UnregisterCallback in the shutdown listener of  nsChannelClassifier may leak because when we shutdown FF, there may be no nsChannelClassifier exists. (Thanks Henry for pointing out)
Therefore we have to use another singleton class to call RegisterCallbackAndCall in ctor and UnregisterCallback in dtor.
Putting this singleton in StaticAutoPtr and using ClearOnShutdown is a proper way to make sure everything is cleared on FF shutdown.
Fixed some nits based on Francois's comment
No longer depends on: 1357898
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review135030

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #49)
> Another way I tried , we use new static variables to store preferences(use
> RegisterCallbackAndCall) and register 1 time for the very first init of all
> nsChannelClassifier (though checking sInited static bool, like we did before
> if (!sIsInited) { sIsInited = true; ...} ), we still have to
> UnregisterCallback when FF shutdown. Calling UnregisterCallback in the
> shutdown listener of  nsChannelClassifier may leak because when we shutdown
> FF, there may be no nsChannelClassifier exists.

I see. You're right, the singleton class is a better way to do this. Let's keep that singleton class approach then.

::: commit-message-b0432:3
(Diff revision 10)
> +Bug 1353853 - Cache preferences when doing channel classify
> +
> +Reading from Preference is called everytime channel is connected

nit: I think this part of the commit message is unnecessary since it's already in a comment in `nsChannelClassifier.cpp`

::: netwerk/base/nsChannelClassifier.cpp:71
(Diff revision 10)
> + * That is not fast and we should cache preference values and reuse them
> + */
> +class CachedPrefs final
> +{
> +public:
> +  static CachedPrefs* GetInstance();

This should presumably return a smart pointer, not a raw one.

::: netwerk/base/nsChannelClassifier.cpp:257
(Diff revision 10)
>      }
>  
>      nsCOMPtr<nsIIOService> ios = do_GetService(NS_IOSERVICE_CONTRACTID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> -    const char ALLOWLIST_EXAMPLE_PREF[] = "channelclassifier.allowlist_example";
> +    bool isAllowListExample = CachedPrefs::GetInstance()->IsAllowListExample();

nit: we probably don't need a variable here and could just put the call to `IsAllowListExample()` inside the `if` statement

::: netwerk/base/nsChannelClassifier.cpp:510
(Diff revision 10)
>             uri->GetSpecOrDefault().get()));
>      }
>      // The classify is running in parent process, no need to give a valid event
>      // target
> -    rv = uriClassifier->Classify(principal, nullptr, sAnnotateChannelEnabled | trackingProtectionEnabled,
> +    bool isAnnotateChannelEnabled = CachedPrefs::GetInstance()->IsAnnotateChannelEnabled();
> +    rv = uriClassifier->Classify(principal, nullptr, isAnnotateChannelEnabled | trackingProtectionEnabled,

Oops, this looks like a bug that was introduced in bug 1141814: https://hg.mozilla.org/mozilla-central/rev/fb18b507a912#l2.117

This should be a logical OR (`||`) since that parameter is a boolean.

Since you're changing that line already, let's fix that bug too.

::: netwerk/base/nsChannelClassifier.cpp:833
(Diff revision 10)
>    nsCOMPtr<nsIURIClassifier> uriClassifier =
>      do_GetService(NS_URICLASSIFIERSERVICE_CONTRACTID, &rv);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  nsAutoCString tables;
> -  Preferences::GetCString("urlclassifier.trackingWhitelistTable", &tables);
> +  nsCString trackingWhitelist = CachedPrefs::GetInstance()->GetTrackingWhiteList();
> +  if (trackingWhitelist.IsEmpty()) {

I would remove the unecessary temporary variable here too.

::: netwerk/base/nsChannelClassifier.cpp:920
(Diff revision 10)
>        // |ShouldEnableTrackingProtection| before.
>        MOZ_ASSERT(mTrackingProtectionEnabled, "Should contain a value.");
>  
>        if (aErrorCode == NS_ERROR_TRACKING_URI &&
>            !mTrackingProtectionEnabled.valueOr(false)) {
> -        if (sAnnotateChannelEnabled) {
> +        bool isAnnotateChannelEnabled = CachedPrefs::GetInstance()->IsAnnotateChannelEnabled();

ditto

::: netwerk/base/nsChannelClassifier.cpp:935
(Diff revision 10)
>            if (httpChannel) {
>              httpChannel->SetIsTrackingResource();
>            }
>          }
>  
> -        if (sLowerNetworkPriority) {
> +        bool isLowerNetworkPriority = CachedPrefs::GetInstance()->IsLowerNetworkPriority();

ditto

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1576
(Diff revision 10)
>  {
>    sUrlClassifierDBService = nullptr;
>  }
>  
> +void
> +AppendTables(const nsCString& aTables, nsCString &outTables)

This is a much better way of doing this. Thanks for cleaning that up!
Attachment #8855697 - Flags: review?(francois) → review-
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review135030

> This should presumably return a smart pointer, not a raw one.

Calling private constructor of class StaticAutoPtr is not allowed
https://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/xpcom/base/StaticPtr.h#76
I think it's ok to use raw pointer here

> I would remove the unecessary temporary variable here too.

Thanks Francois, the string will be used one more time later, should we keep this?
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review135030

> Thanks Francois, the string will be used one more time later, should we keep this?

Sorry I missed that. Yes, we should of course keep it in this case :)
Comment on attachment 8855697 [details]
Bug 1353853 - Cache preferences when doing channel classify

https://reviewboard.mozilla.org/r/127576/#review135122

Looks great. Thanks for your patience with this one Thomas! I appreciate all the work you put into this.
Attachment #8855697 - Flags: review?(francois) → review+
(In reply to François Marier [:francois] from comment #56)
> Looks great. Thanks for your patience with this one Thomas! I appreciate all
> the work you put into this.

Agree!  :)

Thomas, thank you for resolving this so quickly.
Thanks, Francois!  Good job!
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffdb24073101
Cache preferences when doing channel classify r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ffdb24073101
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: