New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 808028 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 809961
issue 810033

Blocking:
issue 651354
issue 812178



Sign in to add a comment

Have num_cores-1 workers since the main thread is always busy

Project Member Reported by gab@chromium.org, Feb 1 2018

Issue description

In renderers: workers are always used to help the main
thread, which is always busy and the highest priority. Reserving a
core for the main thread makes sense (v8 traces show that the additional
worker doesn't get scheduled much by the OS and when it does it
preempts the main thread).

https://docs.google.com/document/d/1bdlWAWeP3j2yo2DYfeok6URqFCrt57yx-nucGMybGGQ/edit#heading=h.rwvxc6ljp9og
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f61636f214c9a53a91906a2ec4b35af511bb5b57

commit f61636f214c9a53a91906a2ec4b35af511bb5b57
Author: Gabriel Charette <gab@chromium.org>
Date: Mon Feb 05 17:50:16 2018

Configure TaskScheduler to use one less worker than cores by default.

Outside of the browser thread, workers are always used to help the main
thread, which is always busy and the highest priority. Reserving a
core for the main thread makes sense (v8 traces show that the additional
worker doesn't get scheduled much by the OS and when it does it
preempts the main thread).

R=fdoray@chromium.org

Bug:  808028 
Change-Id: Iaef9141c6ec271c46ae9c90d9fa625cab5c128e6
Reviewed-on: https://chromium-review.googlesource.com/897528
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534414}
[modify] https://crrev.com/f61636f214c9a53a91906a2ec4b35af511bb5b57/base/task_scheduler/task_scheduler.cc
[modify] https://crrev.com/f61636f214c9a53a91906a2ec4b35af511bb5b57/content/browser/browser_main_loop_unittest.cc
[modify] https://crrev.com/f61636f214c9a53a91906a2ec4b35af511bb5b57/content/common/task_scheduler.cc

Comment 2 by gab@chromium.org, Feb 6 2018

Status: Fixed (was: Started)

Comment 3 by gab@chromium.org, Feb 6 2018

Status: Started (was: Fixed)
Actually, hmmm, turns out we had one too many worker. But then V8 was converting the "num workers" into a "num cores" signal... So now V8 is generating one fewer task than it should... will fix API in V8.
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/cdecc37500e6afb4275c68387da21b6faa5d5a01

commit cdecc37500e6afb4275c68387da21b6faa5d5a01
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Feb 06 16:22:17 2018

[v8::heap] Include main thread in num_tasks computations.

The num_tasks computation has long been based on NumberOfAvailableBackgroundThreads()

We used to have one background worker per core, stealing cycles from
the main thread. I fixed that @ crrev.com/534414. But now this
computation is wrong and generates one less task than it should (one
per worker but the main thread takes task #0 in practice).

Other usage of NumberOfAvailableBackgroundThreads() in V8 seem correct
already so this is the only tweak required.

R=mlippautz@chromium.org

Bug:  chromium:808028 
Change-Id: I784ed9b764017f146931547d30be4a3b180b5a2c
Reviewed-on: https://chromium-review.googlesource.com/904662
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51121}
[modify] https://crrev.com/cdecc37500e6afb4275c68387da21b6faa5d5a01/src/heap/heap.cc
[modify] https://crrev.com/cdecc37500e6afb4275c68387da21b6faa5d5a01/src/heap/mark-compact.cc

Comment 5 by gab@chromium.org, Feb 6 2018

Status: Fixed (was: Started)

Comment 6 by gab@chromium.org, Feb 12 2018

Blockedon: 809961
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/8996e262cc880d6d12ac7dc468964e6313fde023

commit 8996e262cc880d6d12ac7dc468964e6313fde023
Author: Gabriel Charette <gab@chromium.org>
Date: Mon Feb 12 18:07:09 2018

Fix 'num cores' in ConcurrentMarking as well.

This is a follow-up to https://chromium-review.googlesource.com/904662
as I forgot this callsite there. The perf tests still haven't recovered
from decreasing the worker count by 1 to account for main thread
( crbug.com/809961 ) and I assume this line is at fault.

If this is correct, it also indicates ConcurrentMarking as a great
area to focus since a single extra worker appears to be making a
significant difference.

R=mlippautz@chromium.org

Bug:  chromium:809961 ,  chromium:808028 
Change-Id: I9df933a4193fb25ea4e857f589e2164c8a2859b4
Reviewed-on: https://chromium-review.googlesource.com/911670
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51249}
[modify] https://crrev.com/8996e262cc880d6d12ac7dc468964e6313fde023/src/heap/concurrent-marking.cc

Comment 8 by gab@chromium.org, Feb 13 2018

Blockedon: 810033

Comment 9 by gab@chromium.org, Feb 14 2018

Blocking: 812178

Sign in to add a comment