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

Issue 2098020: Jankometer: Generalize the code more. Add better support for monitoring IO thread. (Closed)

Created:
10 years, 7 months ago by willchan no longer on Chromium
Modified:
9 years, 7 months ago
CC:
chromium-reviews
Base URL:
http://src.chromium.org/git/chromium.git
Visibility:
Public.

Description

Jankometer: Generalize the code more. Add better support for monitoring IO thread. Previously, the Jankometer only monitored windows messages on the UI thread (or gtk events). I've added observers for tasks and IO events. This lets us monitor all events on UI & IO threads (UI messages, all Tasks, and IO events). Replaces the JankObserver with a UIJankObserver and an IOJankObserver. Shares common code in JankObserverHelper. The JankObserverHelper and JankWatchdog are generic enough that they can probably move out to chrome/common and be reused by the renderer. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49188

Patch Set 1 #

Total comments: 19

Patch Set 2 : Address comments. #

Total comments: 14

Patch Set 3 : Address darin's comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+496 lines, -168 lines) Patch
M base/message_loop.h View 1 2 9 chunks +60 lines, -18 lines 0 comments Download
M base/message_loop.cc View 1 3 chunks +13 lines, -7 lines 0 comments Download
M base/message_loop_unittest.cc View 1 6 chunks +73 lines, -8 lines 0 comments Download
M base/message_pump_libevent.h View 1 2 6 chunks +60 lines, -26 lines 0 comments Download
M base/message_pump_libevent.cc View 1 2 7 chunks +50 lines, -9 lines 0 comments Download
M base/message_pump_win.h View 1 2 5 chunks +24 lines, -1 line 0 comments Download
M base/message_pump_win.cc View 2 chunks +19 lines, -1 line 0 comments Download
M base/tracked.h View 2 chunks +8 lines, -2 lines 0 comments Download
M chrome/browser/jankometer.cc View 1 3 chunks +189 lines, -96 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
willchan no longer on Chromium
This code looks really old, so I'm not sure who really wrote it. Picked two ...
10 years, 7 months ago (2010-05-24 22:50:09 UTC) #1
willchan no longer on Chromium
[ +brettw who jar says actually wrote most of the jankometer ] On 2010/05/24 22:50:09, ...
10 years, 7 months ago (2010-05-28 01:27:37 UTC) #2
jar (doing other things)
Sorry to be slow in getting a review. Here are some partial notes, and I'll ...
10 years, 7 months ago (2010-05-28 07:00:35 UTC) #3
darin (slow to review)
http://codereview.chromium.org/2098020/diff/1/3 File base/message_loop.h (right): http://codereview.chromium.org/2098020/diff/1/3#newcode72 base/message_loop.h:72: virtual void WillProcessTask(const Task& task) = 0; nit: Task ...
10 years, 7 months ago (2010-05-28 16:24:48 UTC) #4
brettw
This is pretty paged out of my brain. I looked at this some and didn't ...
10 years, 7 months ago (2010-05-28 22:32:48 UTC) #5
willchan no longer on Chromium
I think I've addressed all the comments. http://codereview.chromium.org/2098020/diff/1/2 File base/message_loop.cc (left): http://codereview.chromium.org/2098020/diff/1/2#oldcode593 base/message_loop.cc:593: void MessageLoopForUI::PumpOutPendingPaintMessages() ...
10 years, 6 months ago (2010-06-01 21:18:24 UTC) #6
darin (slow to review)
On Tue, Jun 1, 2010 at 2:18 PM, <willchan@chromium.org> wrote: > I think I've addressed ...
10 years, 6 months ago (2010-06-02 04:45:22 UTC) #7
darin (slow to review)
http://codereview.chromium.org/2098020/diff/12001/13002 File base/message_loop.h (right): http://codereview.chromium.org/2098020/diff/12001/13002#newcode69 base/message_loop.h:69: virtual ~TaskObserver() {} nit: ~TaskObserver should probably be protected ...
10 years, 6 months ago (2010-06-02 04:55:14 UTC) #8
willchan no longer on Chromium
http://codereview.chromium.org/2098020/diff/12001/13002 File base/message_loop.h (right): http://codereview.chromium.org/2098020/diff/12001/13002#newcode69 base/message_loop.h:69: virtual ~TaskObserver() {} On 2010/06/02 04:55:14, darin wrote: > ...
10 years, 6 months ago (2010-06-03 19:01:06 UTC) #9
willchan no longer on Chromium
Ping On 2010/06/03 19:01:06, willchan wrote: > http://codereview.chromium.org/2098020/diff/12001/13002 > File base/message_loop.h (right): > > http://codereview.chromium.org/2098020/diff/12001/13002#newcode69 ...
10 years, 6 months ago (2010-06-07 20:22:00 UTC) #10
darin (slow to review)
LGTM (sorry for the delay; i've been on vacation)
10 years, 6 months ago (2010-06-08 18:24:00 UTC) #11
hamaji
10 years, 6 months ago (2010-06-09 05:30:34 UTC) #12
I've reverted this change as I guessed this caused valgrind failures:

http://build.chromium.org/buildbot/memory/builders/Linux%20Tests%20%28valgrin...
http://build.chromium.org/buildbot/memory/builders/Linux%20Tests%20%28valgrin...

The revert was done in

http://src.chromium.org/viewvc/chrome?view=rev&revision=49225

(I'm sorry, my change description was totally wrong...)

Powered by Google App Engine
This is Rietveld 408576698