Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2084093002:
Use lossy prefs in the net predictor, and update them more frequently (Closed)
|
Created:
4 years, 6 months ago by Charlie Harrison Modified:
4 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, mmenke, eroman Base URL:
https://chromium.googlesource.com/chromium/src.git@predictor_lru Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse lossy prefs in the net predictor, and update them more frequently
This patch changes the predictor to use lossy prefs to sync referrer lists,
and updates them upon receiving the load event from a main frame document.
This fixes an issue where prefs are not updated on unclean shutdown, and
a hang during a synchronous thread hop roundtrip.
BUG=78451, 621604
Committed: https://crrev.com/ee45633faa3fae0320db4b630ec24221e930478e
Cr-Commit-Position: refs/heads/master@{#409017}
Patch Set 1 #Patch Set 2 : Use lossy prefs in the net predictor, and update them more frequently #Patch Set 3 : Fix browser tests #
Total comments: 10
Patch Set 4 : Make saving prefs a little saner and a little insaner #
Total comments: 4
Patch Set 5 : weak factory invalidation + cleanups and comments #Patch Set 6 : ui thread weak factory :/ #
Total comments: 8
Patch Set 7 : bauerb@ review #
Total comments: 5
Patch Set 8 : Don't initialize lists to save with the old values! #Patch Set 9 : update unit tests for new API #
Total comments: 4
Patch Set 10 : kill waitable event references #
Messages
Total messages: 54 (35 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@, do you mind taking a look at this? I'm still a bit unsure about a few things: 1. If we want to persist the predictor DB more often (on document load), do we need to use lossy prefs as an optimization technique? This is the assumption used in this CL, to avoid spamming the disk. 2. Are lossy prefs okay wrt browsing_data_remover.cc? Is there a way to force write a lossy pref? Thanks for the help.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/28 22:07:54, csharrison wrote: > bauerb@, do you mind taking a look at this? I'm still a bit unsure about a few > things: > 1. If we want to persist the predictor DB more often (on document load), do we > need to use lossy prefs as an optimization technique? This is the assumption > used in this CL, to avoid spamming the disk. Yes, I think that would make sense. > 2. Are lossy prefs okay wrt browsing_data_remover.cc? They should be okay -- lossy prefs just don't trigger a commit when they're modified, but that only affects writing to disk, and BrowsingDataRemover modifies preferences in memory, which then schedules a commit. > Is there a way to force write a lossy pref? Yes, there is: PrefService::SchedulePendingLossyWrites(). > Thanks for the help. https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (left): https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:705: if (BrowserThread::CurrentlyOn(BrowserThread::IO)) { Thanks for cleaning this up, BTW! This looks like the code used to access the prefs on the IO thread? That's definitely wrong... https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:679: static void RetainUpdateClosure(ListPrefUpdate* update_startup_list, Move this to an anonymous namespace? https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:687: ListPrefUpdate* update_startup_list = This is not allowed, because ownership of the returned value remains with the PrefService, which could e.g. delete it on the UI thread in the meantime. What I would instead do is this: 1) Read the values from the PrefService and make a DeepCopy() so you actually own them 2) Write to the values on the IO thread 3) Back on the UI thread, write them to the PrefService. You also have to ensure that the PrefService is alive when you're back on the UI thread. How does shutdown of this object work again? Is there a method that is called on the UI thread at shutdown? https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor_tab_helper.cc (right): https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor_tab_helper.cc:60: Predictor* predictor(profile->GetNetworkPredictor()); Nit: I would use assignment here to initialize |predictor|.
Thanks for the review. I'm still a bit stuck on the best path forward for the PostTaskAndReply bit though. https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (left): https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:705: if (BrowserThread::CurrentlyOn(BrowserThread::IO)) { On 2016/07/29 11:07:53, Bernhard Bauer wrote: > Thanks for cleaning this up, BTW! This looks like the code used to access the > prefs on the IO thread? That's definitely wrong... No problem, this also triggers a synchronous thread hop which I think attempts to make the IO thread access safe. Not sure if it *actually* does :/ https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:679: static void RetainUpdateClosure(ListPrefUpdate* update_startup_list, On 2016/07/29 11:07:53, Bernhard Bauer wrote: > Move this to an anonymous namespace? This was changed to actually perform the update, so it's staying a method that needs access to user_prefs_. https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:687: ListPrefUpdate* update_startup_list = On 2016/07/29 11:07:53, Bernhard Bauer wrote: > This is not allowed, because ownership of the returned value remains with the > PrefService, which could e.g. delete it on the UI thread in the meantime. > > What I would instead do is this: > 1) Read the values from the PrefService and make a DeepCopy() so you actually > own them > 2) Write to the values on the IO thread > 3) Back on the UI thread, write them to the PrefService. > > You also have to ensure that the PrefService is alive when you're back on the UI > thread. How does shutdown of this object work again? Is there a method that is > called on the UI thread at shutdown? Thanks this is a good suggestion. We can clear the |user_prefs_| pointer in ShutdownOnUIThread as an indicator that we probably shouldn't be doing any update. The only problem is that it seems like we can't use WeakPtrs for both callbacks in the PostTaskAndReply due to thread safety concerns. Is there a common way to solve this? Obviously I could add another WeakFactory but that seems like overkill. The current patch punts on this issue by using unretained. https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor_tab_helper.cc (right): https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor_tab_helper.cc:60: Predictor* predictor(profile->GetNetworkPredictor()); On 2016/07/29 11:07:53, Bernhard Bauer wrote: > Nit: I would use assignment here to initialize |predictor|. Done.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:687: ListPrefUpdate* update_startup_list = On 2016/07/29 13:42:13, csharrison wrote: > On 2016/07/29 11:07:53, Bernhard Bauer wrote: > > This is not allowed, because ownership of the returned value remains with the > > PrefService, which could e.g. delete it on the UI thread in the meantime. > > > > What I would instead do is this: > > 1) Read the values from the PrefService and make a DeepCopy() so you actually > > own them > > 2) Write to the values on the IO thread > > 3) Back on the UI thread, write them to the PrefService. > > > > You also have to ensure that the PrefService is alive when you're back on the > UI > > thread. How does shutdown of this object work again? Is there a method that is > > called on the UI thread at shutdown? > > Thanks this is a good suggestion. We can clear the |user_prefs_| pointer in > ShutdownOnUIThread as an indicator that we probably shouldn't be doing any > update. The only problem is that it seems like we can't use WeakPtrs for both > callbacks in the PostTaskAndReply due to thread safety concerns. Is there a > common way to solve this? Obviously I could add another WeakFactory but that > seems like overkill. The current patch punts on this issue by using unretained. Hm, that's a tricky one. Like I said, it all depends on the lifetime of this class -- IIRC, on profile destruction ShutdownOnUIThread() is called and then a task is posted to delete this on the IO thread. In that case we don't actually need the weak pointer for posting to the IO thread, as long as we know that ShutdownOnUIThread() hasn't been called (because anything we post to the IO thread at that point is guaranteed to run before the object is destroyed). We do need to watch out when posting back to the UI thread though (which doesn't currently happen yet AFAICT), which might require a WeakPtrFactory, which would invalidate its pointers in ShutdownOnUIThread(). That way at least we won't have more than one WeakPtrFactory :) (Also, documenting the reasoning I'm outlining above would be very good. Hint, hint 😉) https://codereview.chromium.org/2084093002/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:689: base::ListValue* referral_list = Could you store this in a unique_ptr<>? DeepCopy() returns a raw pointer because it's an old method, but we can do better than that now :) https://codereview.chromium.org/2084093002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:715: startup_update->Swap(startup_list); You can just call user_prefs_->Set(key, value). ListPrefUpdate() is for making modifications in-place.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL at the latest patch set, which adds a UI thread weak pointer factory. See comment for reasoning why this is needed. https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:687: ListPrefUpdate* update_startup_list = On 2016/07/29 14:08:14, Bernhard Bauer wrote: > On 2016/07/29 13:42:13, csharrison wrote: > > On 2016/07/29 11:07:53, Bernhard Bauer wrote: > > > This is not allowed, because ownership of the returned value remains with > the > > > PrefService, which could e.g. delete it on the UI thread in the meantime. > > > > > > What I would instead do is this: > > > 1) Read the values from the PrefService and make a DeepCopy() so you > actually > > > own them > > > 2) Write to the values on the IO thread > > > 3) Back on the UI thread, write them to the PrefService. > > > > > > You also have to ensure that the PrefService is alive when you're back on > the > > UI > > > thread. How does shutdown of this object work again? Is there a method that > is > > > called on the UI thread at shutdown? > > > > Thanks this is a good suggestion. We can clear the |user_prefs_| pointer in > > ShutdownOnUIThread as an indicator that we probably shouldn't be doing any > > update. The only problem is that it seems like we can't use WeakPtrs for both > > callbacks in the PostTaskAndReply due to thread safety concerns. Is there a > > common way to solve this? Obviously I could add another WeakFactory but that > > seems like overkill. The current patch punts on this issue by using > unretained. > > Hm, that's a tricky one. Like I said, it all depends on the lifetime of this > class -- IIRC, on profile destruction ShutdownOnUIThread() is called and then a > task is posted to delete this on the IO thread. In that case we don't actually > need the weak pointer for posting to the IO thread, as long as we know that > ShutdownOnUIThread() hasn't been called (because anything we post to the IO > thread at that point is guaranteed to run before the object is destroyed). We do > need to watch out when posting back to the UI thread though (which doesn't > currently happen yet AFAICT), which might require a WeakPtrFactory, which would > invalidate its pointers in ShutdownOnUIThread(). That way at least we won't have > more than one WeakPtrFactory :) > > (Also, documenting the reasoning I'm outlining above would be very good. Hint, > hint 😉) Hm okay so I chatted with mmenke@ about this (cced), and I think there is a problem with using the current weak_factory_ for this, which is used for weak pointers on the IO thread. I don't think there's a way around this, unless we defer saving prefs until the IO thread weak_factory has no more weak pointers. So, I've added another weak factory for UI thread weak pointers. https://codereview.chromium.org/2084093002/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:689: base::ListValue* referral_list = On 2016/07/29 14:08:14, Bernhard Bauer wrote: > Could you store this in a unique_ptr<>? DeepCopy() returns a raw pointer because > it's an old method, but we can do better than that now :) Done. https://codereview.chromium.org/2084093002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:715: startup_update->Swap(startup_list); On 2016/07/29 14:08:14, Bernhard Bauer wrote: > You can just call user_prefs_->Set(key, value). ListPrefUpdate() is for making > modifications in-place. Much better :) done.
cc eroman@ if you want to take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with some last nits. I don't think using a second weak pointer factory is that bad -- at least when we're starting from a point of an object that lives on two threads. https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:154: ui_weak_factory_.reset(new base::WeakPtrFactory<Predictor>(this)); You could just initialize this in the constructor. https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:691: std::unique_ptr<base::ListValue> startup_list = base::WrapUnique( You can just initialize the unique_ptr with the raw pointer. https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:700: // It is safe to use unretained to the task on the IO thread, because this If you still have the |io_weak_factory_|, you could just use a weak pointer from there. I don't really have a strong opinion about it :) https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.h:551: // Weak factory for weak pointers that should be dereferenced on the UI Nit: Empty line before the comment?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison@chromium.org changed reviewers: + mmenke@chromium.org
Thanks a lot. I feel a lot better about this change now. +mmenke explicitly for OWNERS pass. https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:154: ui_weak_factory_.reset(new base::WeakPtrFactory<Predictor>(this)); On 2016/08/01 09:06:24, Bernhard Bauer wrote: > You could just initialize this in the constructor. Done. https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:691: std::unique_ptr<base::ListValue> startup_list = base::WrapUnique( On 2016/08/01 09:06:24, Bernhard Bauer wrote: > You can just initialize the unique_ptr with the raw pointer. Done. https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:700: // It is safe to use unretained to the task on the IO thread, because this On 2016/08/01 09:06:24, Bernhard Bauer wrote: > If you still have the |io_weak_factory_|, you could just use a weak pointer from > there. I don't really have a strong opinion about it :) No, that makes the code much clearer (+robust). No more relying on complex destruction ordering. https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/2084093002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.h:551: // Weak factory for weak pointers that should be dereferenced on the UI On 2016/08/01 09:06:24, Bernhard Bauer wrote: > Nit: Empty line before the comment? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2084093002/diff/120001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/120001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:689: user_prefs_->GetList(prefs::kDnsPrefetchingStartupList)->DeepCopy()); I'm not following this code. WriteDnsPrefetchState calls GetInitialDnsResolutionList, which looks to clear startup_list, so why are we copying stuff to it here? Similarly, SerializeReferrers looks to clear referral_list.
https://codereview.chromium.org/2084093002/diff/120001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/120001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:689: user_prefs_->GetList(prefs::kDnsPrefetchingStartupList)->DeepCopy()); On 2016/08/01 15:11:20, mmenke wrote: > I'm not following this code. WriteDnsPrefetchState calls > GetInitialDnsResolutionList, which looks to clear startup_list, so why are we > copying stuff to it here? Similarly, SerializeReferrers looks to clear > referral_list. Huh, you're right! Which makes me wonder why this was using ListPrefUpdate in the first place... :)
https://codereview.chromium.org/2084093002/diff/120001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/120001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:689: user_prefs_->GetList(prefs::kDnsPrefetchingStartupList)->DeepCopy()); On 2016/08/01 15:18:52, Bernhard Bauer wrote: > On 2016/08/01 15:11:20, mmenke wrote: > > I'm not following this code. WriteDnsPrefetchState calls > > GetInitialDnsResolutionList, which looks to clear startup_list, so why are we > > copying stuff to it here? Similarly, SerializeReferrers looks to clear > > referral_list. > > Huh, you're right! Which makes me wonder why this was using ListPrefUpdate in > the first place... :) Yeah, good catch Matt. I think we can just initialize an empty list here and replace the pref wholesale in UpdatePrefsOnUIThread.
https://codereview.chromium.org/2084093002/diff/120001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/120001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:689: user_prefs_->GetList(prefs::kDnsPrefetchingStartupList)->DeepCopy()); On 2016/08/01 15:20:03, csharrison wrote: > On 2016/08/01 15:18:52, Bernhard Bauer wrote: > > On 2016/08/01 15:11:20, mmenke wrote: > > > I'm not following this code. WriteDnsPrefetchState calls > > > GetInitialDnsResolutionList, which looks to clear startup_list, so why are > we > > > copying stuff to it here? Similarly, SerializeReferrers looks to clear > > > referral_list. > > > > Huh, you're right! Which makes me wonder why this was using ListPrefUpdate in > > the first place... :) > > Yeah, good catch Matt. I think we can just initialize an empty list here and > replace the pref wholesale in UpdatePrefsOnUIThread. Or could just create them on the IO thread. Guess there's not an event we could watch for there to use to know when to save the updated list. Otherwise, we could save ourselves the double hop, too.
https://codereview.chromium.org/2084093002/diff/120001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/120001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:689: user_prefs_->GetList(prefs::kDnsPrefetchingStartupList)->DeepCopy()); On 2016/08/01 15:27:48, mmenke wrote: > On 2016/08/01 15:20:03, csharrison wrote: > > On 2016/08/01 15:18:52, Bernhard Bauer wrote: > > > On 2016/08/01 15:11:20, mmenke wrote: > > > > I'm not following this code. WriteDnsPrefetchState calls > > > > GetInitialDnsResolutionList, which looks to clear startup_list, so why are > > we > > > > copying stuff to it here? Similarly, SerializeReferrers looks to clear > > > > referral_list. > > > > > > Huh, you're right! Which makes me wonder why this was using ListPrefUpdate > in > > > the first place... :) > > > > Yeah, good catch Matt. I think we can just initialize an empty list here and > > replace the pref wholesale in UpdatePrefsOnUIThread. > > Or could just create them on the IO thread. Guess there's not an event we could > watch for there to use to know when to save the updated list. Otherwise, we > could save ourselves the double hop, too. Given the double hop, I think I prefer initializing the list on the UI thread here, as it makes the control flow / data flow explicit.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Okay I've updated the CL. Matt, PTAL thanks again.
LGTM! https://codereview.chromium.org/2084093002/diff/160001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:26: #include "base/synchronization/waitable_event.h" Header no longer needed. https://codereview.chromium.org/2084093002/diff/160001/chrome/browser/net/pre... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/2084093002/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor.h:50: class WaitableEvent; WaitableEvent not needed.
Thanks! https://codereview.chromium.org/2084093002/diff/160001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2084093002/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:26: #include "base/synchronization/waitable_event.h" On 2016/08/01 17:41:25, mmenke wrote: > Header no longer needed. Done. https://codereview.chromium.org/2084093002/diff/160001/chrome/browser/net/pre... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/2084093002/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor.h:50: class WaitableEvent; On 2016/08/01 17:41:25, mmenke wrote: > WaitableEvent not needed. Done.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2084093002/#ps180001 (title: "kill waitable event references")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Use lossy prefs in the net predictor, and update them more frequently This patch changes the predictor to use lossy prefs to sync referrer lists, and updates them upon receiving the load event from a main frame document. This fixes an issue where prefs are not updated on unclean shutdown, and a hang during a synchronous thread hop roundtrip. BUG=78451,621604 ========== to ========== Use lossy prefs in the net predictor, and update them more frequently This patch changes the predictor to use lossy prefs to sync referrer lists, and updates them upon receiving the load event from a main frame document. This fixes an issue where prefs are not updated on unclean shutdown, and a hang during a synchronous thread hop roundtrip. BUG=78451,621604 Committed: https://crrev.com/ee45633faa3fae0320db4b630ec24221e930478e Cr-Commit-Position: refs/heads/master@{#409017} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/ee45633faa3fae0320db4b630ec24221e930478e Cr-Commit-Position: refs/heads/master@{#409017} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
