Skip to content

Commit

Permalink
compute pressure: Increase timeouts and drop uses of t.step_wait()
Browse files Browse the repository at this point in the history
- When we need to wait to verify that an update was _not_ sent, update
  the timeout from 1 second to 3 seconds (and mark tests with
  timeout=long). 1 second is sometimes too little for busy bots such as
  the mac-rel and win-rel ones, which can lead to flakiness or, in this
  case, updates not being delivered for the wrong reason (i.e. the
  browser process just has not had time to process the
  update_virtual_pressure_source() call).

- Introduce a new SyncPressureObserver wrapper class that stores
  multiple pressure updates and allows users to synchronously wait for
  the next one using Promises. This is useful for tests that need to
  send and wait for multiple updates: instead of polling using
  t.step_wait(), these tests can just use this class to wait for a
  Promise to resolve when an update is delivered.

Bug: 347031400
Change-Id: I7880577757d71678e612d912716f4f12020a5fef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5912856
Reviewed-by: Reilly Grant <[email protected]>
Commit-Queue: Reilly Grant <[email protected]>
Auto-Submit: Raphael Kubo Da Costa <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1365092}
  • Loading branch information
Raphael Kubo da Costa authored and chromium-wpt-export-bot committed Oct 7, 2024
1 parent 07001d0 commit 5f9ff49
Show file tree
Hide file tree
Showing 7 changed files with 105 additions and 74 deletions.
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// META: timeout=long
// META: variant=?globalScope=window
// META: script=/resources/testdriver.js
// META: script=/resources/testdriver-vendor.js
Expand Down Expand Up @@ -98,7 +99,7 @@ pressure_test(async t => {
iframe.remove();
await updatePromise;

return new Promise(resolve => t.step_timeout(resolve, 1000));
return new Promise(resolve => t.step_timeout(resolve, 3000));
}, 'PressureObserver on detached frame returns with no callback');

mark_as_done();
42 changes: 18 additions & 24 deletions compute-pressure/compute_pressure_duplicate_updates.https.window.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
// META: timeout=long
// META: variant=?globalScope=window
// META: variant=?globalScope=dedicated_worker
// META: script=/resources/testdriver.js
// META: script=/resources/testdriver-vendor.js
// META: script=/common/utils.js
// META: script=/common/dispatcher/dispatcher.js
// META: script=./resources/common.js
// META: script=./resources/sync-pressure-observer.js

'use strict';

Expand All @@ -14,30 +16,22 @@ pressure_test(async (t) => {
await remove_virtual_pressure_source('cpu');
});

let pressureChanges = [];
const observer = new PressureObserver((changes) => {
pressureChanges = pressureChanges.concat(changes);
});
t.add_cleanup(() => {
observer.disconnect();
});
await observer.observe('cpu', {sampleInterval: 100});
const input = ['critical', 'critical', 'nominal'];
while (input.length != 0) {
await update_virtual_pressure_source('cpu', input.shift());
const currentChangesLength = pressureChanges.length;
await Promise.race([
new Promise((resolve) => {
t.step_timeout(() => resolve('TIMEOUT'), 1000);
}),
t.step_wait(
() => pressureChanges.length === currentChangesLength + 1,
'Wait for new reading'),
]);
}
assert_equals(pressureChanges.length, 2);
assert_equals(pressureChanges[0].state, 'critical');
assert_equals(pressureChanges[1].state, 'nominal');
const syncObserver = new SyncPressureObserver(t);
await syncObserver.observer().observe('cpu', {sampleInterval: 100});

await update_virtual_pressure_source('cpu', 'critical');
await syncObserver.waitForUpdate();
assert_equals(syncObserver.changes()[0][0].state, 'critical');

await update_virtual_pressure_source('cpu', 'critical');
await new Promise(resolve => {t.step_timeout(resolve, 3000)});
assert_equals(syncObserver.changes().length, 1);

await update_virtual_pressure_source('cpu', 'nominal');
await syncObserver.waitForUpdate();
assert_equals(syncObserver.changes()[1][0].state, 'nominal');

assert_equals(syncObserver.changes().length, 2);
}, 'Changes that fail the "has change in data" test are discarded.');

mark_as_done();
28 changes: 12 additions & 16 deletions compute-pressure/compute_pressure_timestamp.https.window.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// META: script=/common/utils.js
// META: script=/common/dispatcher/dispatcher.js
// META: script=./resources/common.js
// META: script=./resources/sync-pressure-observer.js

'use strict';

Expand All @@ -31,28 +32,23 @@ pressure_test(async (t) => {
await remove_virtual_pressure_source('cpu');
});

let pressureChanges = [];
const observer = new PressureObserver((changes) => {
pressureChanges = pressureChanges.concat(changes);
});
t.add_cleanup(() => {
observer.disconnect();
});
const readings = ['critical', 'critical'];
const syncObserver = new SyncPressureObserver(t);

// When disconnect() is called, PressureRecord in [[LastRecordMap]] for cpu
// should be cleared. The effect we observe in this test is the "has change
// in data" algorithm passing with the same state twice.
const states = ['critical', 'critical'];
for (let i = 0; i < states.length; ++i) {
await observer.observe('cpu', {sampleInterval: 500});
await update_virtual_pressure_source('cpu', states[i]);
await t.step_wait(() => pressureChanges.length == i + 1, 'foo');
observer.disconnect();
for (let i = 0; i < readings.length; ++i) {
await syncObserver.observer().observe('cpu', {sampleInterval: 500});
await update_virtual_pressure_source('cpu', readings[i]);
await syncObserver.waitForUpdate();
syncObserver.observer().disconnect();
}

assert_equals(pressureChanges.length, 2);
assert_equals(pressureChanges[0].state, 'critical');
assert_equals(pressureChanges[1].state, 'critical');
const pressureChanges = syncObserver.changes();
assert_equals(pressureChanges.length, readings.length);
assert_equals(pressureChanges[0][0].state, 'critical');
assert_equals(pressureChanges[1][0].state, 'critical');
}, 'disconnect() should update [[LastRecordMap]]');

mark_as_done();
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// META: script=/common/utils.js
// META: script=/common/dispatcher/dispatcher.js
// META: script=./resources/common.js
// META: script=./resources/sync-pressure-observer.js

'use strict';

Expand All @@ -17,23 +18,17 @@ pressure_test(async t => {

const readings = ['nominal', 'fair', 'serious', 'critical'];

const pressureChanges = [];
const observer = new PressureObserver(changes => {
pressureChanges.push(changes);
});
await observer.observe('cpu', {sampleInterval: 250});

let i = 0;
while (pressureChanges.length < 4) {
await update_virtual_pressure_source(
'cpu', readings[i++ % readings.length]);
await t.step_wait(
() => pressureChanges.length >= i,
`At least ${i} readings have been delivered`);
const syncObserver = new SyncPressureObserver(t);
await syncObserver.observer().observe('cpu', {sampleInterval: 250});

for (let i = 0; i < readings.length; ++i) {
await update_virtual_pressure_source('cpu', readings[i]);
await syncObserver.waitForUpdate();
}
observer.disconnect();

assert_equals(pressureChanges.length, 4);
const pressureChanges = syncObserver.changes();
assert_equals(pressureChanges.length, readings.length);

assert_greater_than(pressureChanges[1][0].time, pressureChanges[0][0].time);
assert_greater_than(pressureChanges[2][0].time, pressureChanges[1][0].time);
assert_greater_than(pressureChanges[3][0].time, pressureChanges[2][0].time);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// META: script=/common/utils.js
// META: script=/common/dispatcher/dispatcher.js
// META: script=./resources/common.js
// META: script=./resources/sync-pressure-observer.js

'use strict';

Expand All @@ -15,31 +16,26 @@ pressure_test(async (t) => {
await remove_virtual_pressure_source('cpu');
});

const sampleInterval = 250;
const readings = ['nominal', 'fair', 'serious', 'critical'];

const sampleInterval = 250;
let pressureChanges = [];
const observer = new PressureObserver((changes) => {
pressureChanges = pressureChanges.concat(changes);
});
t.add_cleanup(() => observer.disconnect());
observer.observe('cpu', {sampleInterval});

for (let i = 0; i < 4;) {
await update_virtual_pressure_source(
'cpu', readings[i++ % readings.length]);
await t.step_wait(
() => pressureChanges.length === i,
`At least ${i} readings have been delivered`);
const syncObserver = new SyncPressureObserver(t);
await syncObserver.observer().observe('cpu', {sampleInterval});

for (let i = 0; i < readings.length; ++i) {
await update_virtual_pressure_source('cpu', readings[i]);
await syncObserver.waitForUpdate();
}

assert_equals(pressureChanges.length, 4);
const pressureChanges = syncObserver.changes();
assert_equals(pressureChanges.length, readings.length);

assert_greater_than_equal(
pressureChanges[1].time - pressureChanges[0].time, sampleInterval);
pressureChanges[1][0].time - pressureChanges[0][0].time, sampleInterval);
assert_greater_than_equal(
pressureChanges[2].time - pressureChanges[1].time, sampleInterval);
pressureChanges[2][0].time - pressureChanges[1][0].time, sampleInterval);
assert_greater_than_equal(
pressureChanges[3].time - pressureChanges[2].time, sampleInterval);
pressureChanges[3][0].time - pressureChanges[2][0].time, sampleInterval);
}, 'Faster collector: Timestamp difference between two changes should be higher or equal to the observer sample rate');

mark_as_done();
46 changes: 46 additions & 0 deletions compute-pressure/resources/sync-pressure-observer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Wrapper around a PressureObserver that:
// 1. Receives and stores multiple updates.
// 2. Allows callers to synchronously wait for an update.
//
// Usage:
// const syncObserver = new SyncPressureObserver(t);
// await syncObserver.observer().observe('cpu');
// await update_virtual_pressure_source(..);
// await syncObserver.waitForUpdate();
// const changes = syncObserver.changes();
// assert_equals(changes[0][0].state, 'nominal');
class SyncPressureObserver {
#observer = null;
#changes = [];

#promisesWithResolver = [Promise.withResolvers()];
#currentPromisePosition = 0;
#currentResolvePosition = 0;

constructor(t) {
this.#observer = new PressureObserver(changes => {
this.#changes.push(changes);

if (this.#currentResolvePosition === this.#promisesWithResolver.length) {
this.#promisesWithResolver.push(Promise.withResolvers());
}
this.#promisesWithResolver[this.#currentResolvePosition++].resolve();
});
t.add_cleanup(() => {this.#observer.disconnect()});
}

changes() {
return this.#changes;
}

observer() {
return this.#observer;
}

async waitForUpdate() {
if (this.#currentPromisePosition === this.#promisesWithResolver.length) {
this.#promisesWithResolver.push(Promise.withResolvers());
}
await this.#promisesWithResolver[this.#currentPromisePosition++].promise;
}
};
3 changes: 3 additions & 0 deletions compute-pressure/resources/worker-support.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ importScripts('/resources/testharness.js');
importScripts('/common/utils.js');
importScripts('/common/dispatcher/dispatcher.js');

// Only used by some tests.
importScripts('/compute-pressure/resources/sync-pressure-observer.js');

function send_message(message) {
return new Promise((resolve, reject) => {
const id = token();
Expand Down

0 comments on commit 5f9ff49

Please sign in to comment.