Closed
Bug 1353853
Opened 7 years ago
Closed 7 years ago
nsUrlClassifierDBService::BuildTables should use cached preferences
Categories
(Toolkit :: Safe Browsing, enhancement, P1)
Toolkit
Safe Browsing
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: kanru, Assigned: tnguyen)
References
Details
Attachments
(1 file, 2 obsolete files)
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.
Comment 1•7 years ago
|
||
Henry, any chance you could take a look at this one please?
Flags: needinfo?(hchang)
Comment 2•7 years ago
|
||
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)
Updated•7 years ago
|
Assignee: nobody → hchang
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee | ||
Updated•7 years ago
|
Assignee: hchang → tnguyen
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: 8XRguvmbM44
Reporter | ||
Comment 4•7 years ago
|
||
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.
Assignee | ||
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
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().
Comment 7•7 years ago
|
||
(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
Assignee | ||
Comment 8•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8855698 -
Flags: review?(hchang)
Attachment #8855698 -
Flags: review?(francois)
Assignee | ||
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
mozreview-review |
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 13•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 15•7 years ago
|
||
>
> 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 16•7 years ago
|
||
mozreview-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 17•7 years ago
|
||
mozreview-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-
Comment 18•7 years ago
|
||
Also, we can probably merge these two patches together. They are essentially doing the same thing and they're both quite small.
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review |
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
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8855698 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
(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
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Comment 24•7 years ago
|
||
mozreview-review |
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 25•7 years ago
|
||
mozreview-review |
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 26•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 27•7 years ago
|
||
(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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•7 years ago
|
||
> 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
Comment 30•7 years ago
|
||
Thanks Thomas! I don't find any issue more. We should be good to ask for Francois's review now :)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8cfcb1f3bdb6b50bdda6d93c961665b73b84bee
Assignee | ||
Comment 33•7 years ago
|
||
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)
Comment 34•7 years ago
|
||
(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!
Assignee | ||
Comment 35•7 years ago
|
||
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 *
Assignee | ||
Comment 36•7 years ago
|
||
duplicated |
https://treeherder.mozilla.org/#/jobs?repo=try&revision=704f405bf2bc493bee680b15437650290cd716a1
Assignee | ||
Comment 37•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=704f405bf2bc493bee680b15437650290cd716a1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 40•7 years ago
|
||
(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
Comment hidden (duplicate, obsolete) |
Assignee | ||
Comment 42•7 years ago
|
||
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?
Comment 43•7 years ago
|
||
(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 44•7 years ago
|
||
mozreview-review |
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-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 46•7 years ago
|
||
Changed to the first approach.
Assignee | ||
Comment 47•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5100a542bf3eac75807544366c3a43d9b531b80
Comment 48•7 years ago
|
||
mozreview-review |
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-
Updated•7 years ago
|
Assignee | ||
Comment 49•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 51•7 years ago
|
||
Fixed some nits based on Francois's comment
Comment 52•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 53•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 55•7 years ago
|
||
mozreview-review-reply |
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 56•7 years ago
|
||
mozreview-review |
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+
Comment 57•7 years ago
|
||
(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!
Assignee | ||
Comment 58•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0412c81c3e58e3c229aefad74a46ac6800bc29a4
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 59•7 years ago
|
||
Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ffdb24073101 Cache preferences when doing channel classify r=francois
Keywords: checkin-needed
Comment 60•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ffdb24073101
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•