Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446603002:
Enable SequencedWorkerPool to TaskScheduler redirection in testing config. (Closed)
DescriptionEnable SequencedWorkerPool to TaskScheduler redirection in testing config.
With this CL, redirection of SequencedWorkerPools to TaskScheduler will
be tested on bots.
Before a similar CL https://codereview.chromium.org/2353973002/ landed,
SequencedWorkerPool tasks didn't generate tracing events. With
redirection to TaskScheduler enabled, the same tasks started to generate
tracing events. That affected the battor.power_cases/cpu_time_percentage_max
graph https://crbug.com/656629 and the CL was reverted. Now that
SequencedWorkerPool tasks generate tracing events
https://codereview.chromium.org/2429863002/, enabling redirection
to TaskScheduler shouldn't affect any performance graph. Note that the
battor.power_cases/cpu_time_percentage_max graph didn't go up when tracing
was enabled in SequencedWorkerPools because of
https://codereview.chromium.org/2420133002/ which reduced the CPU
consumption of a task running in the blocking pool.
BUG=622400
Committed: https://crrev.com/9b3162c323b6529cbba17fb419129fea26b75257
Cr-Commit-Position: refs/heads/master@{#427375}
Patch Set 1 #
Total comments: 2
Patch Set 2 : CR gab #7 (remove default) #Messages
Total messages: 20 (11 generated)
The CQ bit was checked by fdoray@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.
fdoray@chromium.org changed reviewers: + gab@chromium.org
PTAL
Woot, lgtm w/ comment Was the regression properly attributed to the SWP in https://codereview.chromium.org/2429863002/? https://codereview.chromium.org/2446603002/diff/1/testing/variations/fieldtri... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2446603002/diff/1/testing/variations/fieldtri... testing/variations/fieldtrial_testing_config.json:211: "name": "Enabled", I think the first group is the one that runs by default (i.e. you need to replace "Default" above with this)
re: Was the regression properly attributed to the SWP in https://codereview.chromium.org/2429863002/? The graph did recover after redirection to TaskScheduler was reverted. I did not go up after tracing was enabled in SequencedWorkerPool. I believe this is because of https://codereview.chromium.org/2420133002/ Indeed, if you look at traces in the range where TaskScheduler was enabled, you can see that a lot of time was consumed by a ScanNow task from enumerate_modules_model.cc. https://codereview.chromium.org/2446603002/diff/1/testing/variations/fieldtri... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2446603002/diff/1/testing/variations/fieldtri... testing/variations/fieldtrial_testing_config.json:211: "name": "Enabled", On 2016/10/24 19:51:29, gab wrote: > I think the first group is the one that runs by default (i.e. you need to > replace "Default" above with this) Done.
Description was changed from ========== Enable SequencedWorkerPool to TaskScheduler redirection in testing config. With this CL, redirection of SequencedWorkerPools to TaskScheduler will be tested on bots. Before a similar CL https://codereview.chromium.org/2353973002/ landed, SequencedWorkerPool tasks didn't generate tracing events. With redirection to TaskScheduler enabled, the same tasks generated tracing events. That affected the battor.power_cases/cpu_time_percentage_max graph https://crbug.com/656629 and the CL was reverted. Now that SequencedWorkerPool tasks generate tracing events https://codereview.chromium.org/2429863002/, enabling redirection to TaskScheduler shouldn't affect any performance graph. BUG=622400 ========== to ========== Enable SequencedWorkerPool to TaskScheduler redirection in testing config. With this CL, redirection of SequencedWorkerPools to TaskScheduler will be tested on bots. Before a similar CL https://codereview.chromium.org/2353973002/ landed, SequencedWorkerPool tasks didn't generate tracing events. With redirection to TaskScheduler enabled, the same tasks started to generate tracing events. That affected the battor.power_cases/cpu_time_percentage_max graph https://crbug.com/656629 and the CL was reverted. Now that SequencedWorkerPool tasks generate tracing events https://codereview.chromium.org/2429863002/, enabling redirection to TaskScheduler shouldn't affect any performance graph. Note that the battor.power_cases/cpu_time_percentage_max graph didn't go up when tracing was enabled in SequencedWorkerPools because of https://codereview.chromium.org/2420133002/ which reduced the CPU consumption of a task running in the blocking pool. BUG=622400 ==========
fdoray@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow@: PTAL re gab@: Was the regression properly attributed to the SWP in https://codereview.chromium.org/2429863002/? The graph did recover after redirection to TaskScheduler was reverted. I did not go up after tracing was enabled in SequencedWorkerPool. I believe this is because of https://codereview.chromium.org/2420133002/ Indeed, if you look at traces in the range where TaskScheduler was enabled, you can see that a lot of time was consumed by a ScanNow task from enumerate_modules_model.cc.
lgtm
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2446603002/#ps20001 (title: "CR gab #7 (remove default)")
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.
Description was changed from ========== Enable SequencedWorkerPool to TaskScheduler redirection in testing config. With this CL, redirection of SequencedWorkerPools to TaskScheduler will be tested on bots. Before a similar CL https://codereview.chromium.org/2353973002/ landed, SequencedWorkerPool tasks didn't generate tracing events. With redirection to TaskScheduler enabled, the same tasks started to generate tracing events. That affected the battor.power_cases/cpu_time_percentage_max graph https://crbug.com/656629 and the CL was reverted. Now that SequencedWorkerPool tasks generate tracing events https://codereview.chromium.org/2429863002/, enabling redirection to TaskScheduler shouldn't affect any performance graph. Note that the battor.power_cases/cpu_time_percentage_max graph didn't go up when tracing was enabled in SequencedWorkerPools because of https://codereview.chromium.org/2420133002/ which reduced the CPU consumption of a task running in the blocking pool. BUG=622400 ========== to ========== Enable SequencedWorkerPool to TaskScheduler redirection in testing config. With this CL, redirection of SequencedWorkerPools to TaskScheduler will be tested on bots. Before a similar CL https://codereview.chromium.org/2353973002/ landed, SequencedWorkerPool tasks didn't generate tracing events. With redirection to TaskScheduler enabled, the same tasks started to generate tracing events. That affected the battor.power_cases/cpu_time_percentage_max graph https://crbug.com/656629 and the CL was reverted. Now that SequencedWorkerPool tasks generate tracing events https://codereview.chromium.org/2429863002/, enabling redirection to TaskScheduler shouldn't affect any performance graph. Note that the battor.power_cases/cpu_time_percentage_max graph didn't go up when tracing was enabled in SequencedWorkerPools because of https://codereview.chromium.org/2420133002/ which reduced the CPU consumption of a task running in the blocking pool. BUG=622400 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Enable SequencedWorkerPool to TaskScheduler redirection in testing config. With this CL, redirection of SequencedWorkerPools to TaskScheduler will be tested on bots. Before a similar CL https://codereview.chromium.org/2353973002/ landed, SequencedWorkerPool tasks didn't generate tracing events. With redirection to TaskScheduler enabled, the same tasks started to generate tracing events. That affected the battor.power_cases/cpu_time_percentage_max graph https://crbug.com/656629 and the CL was reverted. Now that SequencedWorkerPool tasks generate tracing events https://codereview.chromium.org/2429863002/, enabling redirection to TaskScheduler shouldn't affect any performance graph. Note that the battor.power_cases/cpu_time_percentage_max graph didn't go up when tracing was enabled in SequencedWorkerPools because of https://codereview.chromium.org/2420133002/ which reduced the CPU consumption of a task running in the blocking pool. BUG=622400 ========== to ========== Enable SequencedWorkerPool to TaskScheduler redirection in testing config. With this CL, redirection of SequencedWorkerPools to TaskScheduler will be tested on bots. Before a similar CL https://codereview.chromium.org/2353973002/ landed, SequencedWorkerPool tasks didn't generate tracing events. With redirection to TaskScheduler enabled, the same tasks started to generate tracing events. That affected the battor.power_cases/cpu_time_percentage_max graph https://crbug.com/656629 and the CL was reverted. Now that SequencedWorkerPool tasks generate tracing events https://codereview.chromium.org/2429863002/, enabling redirection to TaskScheduler shouldn't affect any performance graph. Note that the battor.power_cases/cpu_time_percentage_max graph didn't go up when tracing was enabled in SequencedWorkerPools because of https://codereview.chromium.org/2420133002/ which reduced the CPU consumption of a task running in the blocking pool. BUG=622400 Committed: https://crrev.com/9b3162c323b6529cbba17fb419129fea26b75257 Cr-Commit-Position: refs/heads/master@{#427375} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9b3162c323b6529cbba17fb419129fea26b75257 Cr-Commit-Position: refs/heads/master@{#427375}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2488513003/ by fdoray@chromium.org. The reason for reverting is: This caused a 1.1% regression in system_health.memory_mobile. https://crbug.com/661520. |
