Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1141913002:
Make X11 power save suspension calls asynchronous. (Closed)
|
Created:
5 years, 7 months ago by Kevin M Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, Randy Smith (Not in Mondays), Mike Mammarella Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake D-bus power save suspension calls asynchronous.
On some systems with buggy power managers, calls to the power
manager would result in long pauses that would block the FILE thread.
This yielded degraded performance for other browser tasks
scheduled on the same thread.
BUG=173354
R=miu@chromium.org,rdsmith@chromium.org
Committed: https://crrev.com/5c50887f1372b84bef43945652e405cc1b594020
Cr-Commit-Position: refs/heads/master@{#330210}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Code review feedback, rename "unblock_enqueued" to "enqueue_unblock" for consistency. #Messages
Total messages: 19 (7 generated)
rdsmith@chromium.org changed reviewers: + mdm@chromium.org - rdsmith@chromium.org
I really think Mike's the right person to review this.
marshallk@google.com changed reviewers: + marshallk@google.com
kmarshall@chromium.org changed reviewers: + wez@chromium.org - miu@chromium.org
-miu, +wez
miu@chromium.org changed reviewers: + miu@chromium.org
lgtm % nits: https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... File content/browser/power_save_blocker_x11.cc (left): https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... content/browser/power_save_blocker_x11.cc:288: inhibit_cookie_ = 0; nit: Either leave this in, or adjust the header file comment for this member. https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... File content/browser/power_save_blocker_x11.cc (right): https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... content/browser/power_save_blocker_x11.cc:297: unblock_enqueued_ = true; Should you DCHECK(!unblock_enqueued) before this line?
kmarshall@chromium.org changed reviewers: - wez@chromium.org
kmarshall@chromium.org changed reviewers: + avi@chromium.org - mdm@chromium.org
Added avi to the reviewer list. Removed mdm from reviewers - it looks like he's not an active Chromium contributor anymore? (last commit was back in 2012) https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... File content/browser/power_save_blocker_x11.cc (left): https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... content/browser/power_save_blocker_x11.cc:288: inhibit_cookie_ = 0; On 2015/05/15 18:43:31, miu wrote: > nit: Either leave this in, or adjust the header file comment for this member. Done. https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... File content/browser/power_save_blocker_x11.cc (right): https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... content/browser/power_save_blocker_x11.cc:297: unblock_enqueued_ = true; On 2015/05/15 18:43:31, miu wrote: > Should you DCHECK(!unblock_enqueued) before this line? Good idea, done.
On 2015/05/15 18:55:26, Kevin M_ooo_until_June_1 wrote: > Added avi to the reviewer list. > > Removed mdm from reviewers - it looks like he's not an active Chromium > contributor anymore? (last commit was back in 2012) > > https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... > File content/browser/power_save_blocker_x11.cc (left): > > https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... > content/browser/power_save_blocker_x11.cc:288: inhibit_cookie_ = 0; > On 2015/05/15 18:43:31, miu wrote: > > nit: Either leave this in, or adjust the header file comment for this member. > > Done. > > https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... > File content/browser/power_save_blocker_x11.cc (right): > > https://codereview.chromium.org/1141913002/diff/1/content/browser/power_save_... > content/browser/power_save_blocker_x11.cc:297: unblock_enqueued_ = true; > On 2015/05/15 18:43:31, miu wrote: > > Should you DCHECK(!unblock_enqueued) before this line? > > Good idea, done. I have no idea how this is supposed to go. You have my content stamp lgtm, and if you're satisfied that you have an area expert's OK, commit.
Still lgtm. avi: It looks like this is one of those cases where there is no expert left around and we're stuck with asking OWNERS that aren't experts on the code for approval. Nevertheless, there's nothing complicated going on here. I feel Kevin understood and researched this problem sufficiently, and we discussed it and a few alternative approaches (e.g., using the blocking pool). IMO, the small amount of complexity added here is appropriate to resolve the freezing issue users are experiencing.
The CQ bit was checked by miu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1141913002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5c50887f1372b84bef43945652e405cc1b594020 Cr-Commit-Position: refs/heads/master@{#330210} |
