Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(7)

Issue 965503002: Filter out redundant jumplist updates. (Closed)

Created:
5 years, 10 months ago by brucedawson
Modified:
5 years, 6 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Delay jumplist updates, filter redundant updates, and instrument updates. Filter out redundant jumplist updates by ignoring provably pointless updates and posting the update task with a delay to make more updates redundant. A delay of 3500 ms was chosen because it should be short enough to not noticeably affect the freshness of the jumplist, but long enough to let more redundant requests arrive. A delay of 3500 ms also guarantees (assuming a typical update time of ~500 ms) that the browser will not spend a majority of its time updating the jumplist. This change also adds Chrome tracing events to JumpList::PostRunUpdate and JumpList::DeferredRunUpdate for testing and easier diagnosis of update request storms. There have been reports for years of excess updates to the Windows 7+ jumplist icons. One source of these problems is that JumpList::PostRunUpdate may be called multiple times, queueing up a long series of redundant updates. A modest version of this was caught on my machine on the new-tab-page where certain icons that fail to download trigger update requests every time. Another version of this has been inferred from ETW trace data from a customer. This showed JumpList::RunUpdateOnFileThread being called about 70 times over a 33 s period, with no gaps in-between. The simplest explanation for this behavior is queueing of redundant update requests. Note that this data shows that each JumpListUpdate call consumes almost half a second, on a beefy machine with lots of memory, many cores, and an SSD. In that customer's pathological scenario we will now go from having 70+ jumplist updates to having just one, and we will be able to better investigate the cause. Other scenarios that have been reported include closing all tabs during browser shutdown but I have not reproduced this. It is worth separately investigating why JumpListUpdate is so expensive (a FlushFileBuffers call for each icon is a problem) and why these redundant calls are happening (NTP is one known cause, the customer's calls appear to originate from syncer::GetSessionName) but coalescing the redundant calls is still worthwhile. While this change will not fix the related problem of ending up with thousands of jumplist icon files it should reduce the number of files created. TEST=This change can be tested by going to chrome://tracing, clicking Record, checking 'browser' only, and then clicking record. Then close a few tabs in quick succession and wait five seconds before stopping the trace. A search of the trace for JumpList should show a JumpList::PostRunUpdate event for each tab that was closed, and a JumpList::DeferredRunUpdate for the actual update that occurs ~3.5 s after the last JumpList::PostRunUpdate. Bug 498987 tracks adding automated tests. BUG=40407, 179576, 498987 Committed: https://crrev.com/c539ec64212e2a08e4b15c4ed29072e1d4f9fcab Cr-Commit-Position: refs/heads/master@{#334015}

Patch Set 1 #

Patch Set 2 : Adding delay to posting of jumplistupdate requests. #

Patch Set 3 : Cleanup comments and delay amount. #

Total comments: 5

Patch Set 4 : Change implementation to use a OneShotTimer #

Patch Set 5 : Fix some accumulated glitches #

Total comments: 6

Patch Set 6 : Fixing formatting nits. #

Patch Set 7 : Add Chrome tracing to JumpList for testing and diagnostic purposes. #

Total comments: 1

Patch Set 8 : Changed from ui to browser category #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -0 lines) Patch
M chrome/browser/jumplist_win.h View 1 2 3 4 3 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/jumplist_win.cc View 1 2 3 4 5 6 7 3 chunks +21 lines, -0 lines 0 comments Download

Messages

Total messages: 28 (4 generated)
brucedawson
5 years, 10 months ago (2015-02-26 19:55:40 UTC) #2
jochen (gone - plz use gerrit)
is it possible to add a test for this?
5 years, 9 months ago (2015-02-27 09:06:54 UTC) #3
brucedawson
I'm not sure what it would test - and I can't find existing jumplist tests ...
5 years, 9 months ago (2015-02-27 17:58:48 UTC) #4
jochen (gone - plz use gerrit)
the test could just test what you manually verified - queue a bunch of requests ...
5 years, 9 months ago (2015-03-03 13:38:31 UTC) #5
brucedawson
I'm waiting on more customer data. It now appears that the update requests and actions ...
5 years, 9 months ago (2015-03-05 00:24:26 UTC) #6
brucedawson
Cleanup comments and delay amount.
5 years, 8 months ago (2015-03-30 23:35:51 UTC) #7
brucedawson
PTAL. I've enhanced this change to include a delay, to make the filtering more effective, ...
5 years, 8 months ago (2015-03-30 23:42:12 UTC) #8
jochen (gone - plz use gerrit)
the CL description should have a summary as first line what about a test? https://codereview.chromium.org/965503002/diff/40001/chrome/browser/jumplist_win.cc ...
5 years, 8 months ago (2015-03-31 10:28:52 UTC) #9
gab
Drive-by, also see update offline (since it talks about UMA stats). https://codereview.chromium.org/965503002/diff/40001/chrome/browser/jumplist_win.h File chrome/browser/jumplist_win.h (right): ...
5 years, 8 months ago (2015-03-31 13:10:14 UTC) #11
gab
Also, I think it'd be preferable to be using a timer like in ImportantFileWriter::ScheduleWrite() does ...
5 years, 8 months ago (2015-03-31 13:25:53 UTC) #12
brucedawson
PTAL I responded to the feedback and reworked this to use a OneShotTimer. I also ...
5 years, 6 months ago (2015-06-03 17:57:16 UTC) #13
gab
Awesome :-), lgtm w/ a few style nits. Are there bugs opened for the follow-ups ...
5 years, 6 months ago (2015-06-03 19:03:51 UTC) #14
brucedawson
Thanks for the feedback. Nits addressed. I'm not planning to close either bug so I ...
5 years, 6 months ago (2015-06-03 23:27:30 UTC) #15
brucedawson
Publishing my 'Done' comments. jochen@, can you take a look? https://codereview.chromium.org/965503002/diff/80001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/965503002/diff/80001/chrome/browser/jumplist_win.cc#newcode50 ...
5 years, 6 months ago (2015-06-04 17:11:59 UTC) #16
jochen (gone - plz use gerrit)
can we have a test please?
5 years, 6 months ago (2015-06-05 12:11:45 UTC) #17
brucedawson
I'm a huge fan of tests but I'm not sure I agree about the value-prop ...
5 years, 6 months ago (2015-06-05 17:19:17 UTC) #18
jochen (gone - plz use gerrit)
On 2015/06/05 at 17:19:17, brucedawson wrote: > I'm a huge fan of tests but I'm ...
5 years, 6 months ago (2015-06-08 09:30:37 UTC) #19
brucedawson
PTAL. I added Chrome tracing events which make manual testing of jumplist updates much easier, ...
5 years, 6 months ago (2015-06-10 21:06:52 UTC) #20
jochen (gone - plz use gerrit)
thx, lgtm
5 years, 6 months ago (2015-06-11 12:44:07 UTC) #21
gab
lgtm w/ suggestion for category change https://codereview.chromium.org/965503002/diff/120001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/965503002/diff/120001/chrome/browser/jumplist_win.cc#newcode447 chrome/browser/jumplist_win.cc:447: TRACE_EVENT0("ui", "JumpList::PostRunUpdate"); Not ...
5 years, 6 months ago (2015-06-11 17:49:24 UTC) #22
brucedawson
Category change made, and description updated appropriately. Committing now.
5 years, 6 months ago (2015-06-11 18:05:42 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/965503002/140001
5 years, 6 months ago (2015-06-11 18:07:09 UTC) #26
commit-bot: I haz the power
Committed patchset #8 (id:140001)
5 years, 6 months ago (2015-06-11 19:45:15 UTC) #27
commit-bot: I haz the power
5 years, 6 months ago (2015-06-11 19:46:09 UTC) #28
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/c539ec64212e2a08e4b15c4ed29072e1d4f9fcab
Cr-Commit-Position: refs/heads/master@{#334015}

Powered by Google App Engine
This is Rietveld 408576698