Navigation Menu

Skip to content

Commit

Permalink
[ComputePressure] Remove rate-limiting code from PressureServiceImpl
Browse files Browse the repository at this point in the history 
"passes rate test" steps [1] are already implemented in
PressureObserver in the CL [2], so there is no need to do
rate-limit in PressureServiceImpl.

[1] https://wicg.github.io/compute-pressure/#dfn-passes-rate-test
[2] https://chromium-review.googlesource.com/c/chromium/src/+/3932058

Bug: 1385588
Change-Id: Iffa87b0d144aa1eb656bcc5c3718cc8154f8ddbc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4032655
Reviewed-by: Raphael Kubo Da Costa <raphael.kubo.da.costa@intel.com>
Commit-Queue: Wei4 Wang <wei4.wang@intel.com>
Cr-Commit-Position: refs/heads/main@{#1073890}
  • Loading branch information
@wangw-1991
wangw-1991 authored and Chromium LUCI CQ committed Nov 21, 2022
1 parent e9bcc6a commit 00065de
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 80 deletions.
33 changes: 8 additions & 25 deletions content/browser/compute_pressure/pressure_service_impl.cc
View file
Expand Up @@ -18,8 +18,6 @@

namespace content {

constexpr base::TimeDelta PressureServiceImpl::kDefaultVisibleObserverRateLimit;

// static
void PressureServiceImpl::Create(
RenderFrameHost* render_frame_host,
Expand All @@ -30,19 +28,12 @@ void PressureServiceImpl::Create(
return;
}

if (!GetForCurrentDocument(render_frame_host)) {
CreateForCurrentDocument(render_frame_host,
kDefaultVisibleObserverRateLimit);
}

GetForCurrentDocument(render_frame_host)->BindReceiver(std::move(receiver));
GetOrCreateForCurrentDocument(render_frame_host)
->BindReceiver(std::move(receiver));
}

PressureServiceImpl::PressureServiceImpl(
RenderFrameHost* render_frame_host,
base::TimeDelta visible_observer_rate_limit)
: DocumentUserData<PressureServiceImpl>(render_frame_host),
visible_observer_rate_limit_(visible_observer_rate_limit) {
PressureServiceImpl::PressureServiceImpl(RenderFrameHost* render_frame_host)
: DocumentUserData<PressureServiceImpl>(render_frame_host) {
DCHECK(render_frame_host);
}

Expand Down Expand Up @@ -110,27 +101,22 @@ void PressureServiceImpl::PressureStateChanged(
device::mojom::PressureUpdatePtr update) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

// TODO(jsbell): Rate-limit observers in non-visible frames instead of
// cutting off their updates completely.
if (last_reported_update_ &&
update->timestamp - last_reported_update_->timestamp <
visible_observer_rate_limit_) {
return;
}

// TODO(crbug.com/1385588): Remove this when "passes privacy test" steps are
// implemented.
if (!render_frame_host().IsActive()) {
// TODO(jsbell): Is it safe to disconnect observers in this state?
return;
}

// TODO(crbug.com/1385588): Remove this when "passes privacy test" steps are
// implemented.
if (render_frame_host().GetVisibilityState() !=
blink::mojom::PageVisibilityState::kVisible) {
// TODO(jsbell): Rate-limit observers in non-visible frames instead of
// cutting off their updates completely.
return;
}

last_reported_update_ = update.Clone();
observer_->OnUpdate(update.Clone());
}

Expand Down Expand Up @@ -166,9 +152,6 @@ void PressureServiceImpl::ResetObserverState() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

observer_.reset();

// Setting to an invalid value, so any state is considered an update.
last_reported_update_ = nullptr;
}

DOCUMENT_USER_DATA_KEY_IMPL(PressureServiceImpl);
Expand Down
18 changes: 1 addition & 17 deletions content/browser/compute_pressure/pressure_service_impl.h
View file
Expand Up @@ -30,9 +30,6 @@ class CONTENT_EXPORT PressureServiceImpl
public device::mojom::PressureClient,
public DocumentUserData<PressureServiceImpl> {
public:
static constexpr base::TimeDelta kDefaultVisibleObserverRateLimit =
base::Seconds(1);

static void Create(
RenderFrameHost* render_frame_host,
mojo::PendingReceiver<blink::mojom::PressureService> receiver);
Expand All @@ -54,8 +51,7 @@ class CONTENT_EXPORT PressureServiceImpl
void PressureStateChanged(device::mojom::PressureUpdatePtr update) override;

private:
PressureServiceImpl(RenderFrameHost* render_frame_host,
base::TimeDelta visible_observer_rate_limit);
explicit PressureServiceImpl(RenderFrameHost* render_frame_host);

void OnObserverRemoteDisconnected();

Expand All @@ -68,18 +64,6 @@ class CONTENT_EXPORT PressureServiceImpl

SEQUENCE_CHECKER(sequence_checker_);

// The minimum delay between two Update() calls for observers belonging to
// the frame.
const base::TimeDelta visible_observer_rate_limit_;

// The update that was last reported to this frame's observers.
//
// Stored to avoid sending updates when the underlying compute pressure state
// changes, but quantization produces the same values that were reported in
// the last update.
device::mojom::PressureUpdatePtr last_reported_update_
GUARDED_BY_CONTEXT(sequence_checker_) = nullptr;

// Callback from |receiver_| is passed to |remote_| and the Receiver
// should be destroyed first so that the callback is invalidated before
// being discarded.
Expand Down
50 changes: 12 additions & 38 deletions content/browser/compute_pressure/pressure_service_impl_unittest.cc
View file
Expand Up @@ -13,6 +13,7 @@
#include "base/test/bind.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/test_future.h"
#include "base/time/time.h"
#include "content/browser/renderer_host/render_frame_host_impl.h"
#include "content/public/test/navigation_simulator.h"
#include "content/test/test_render_view_host.h"
Expand All @@ -34,8 +35,7 @@ using device::mojom::PressureUpdate;

namespace {

constexpr base::TimeDelta kRateLimit =
PressureServiceImpl::kDefaultVisibleObserverRateLimit;
constexpr base::TimeDelta kSampleInterval = base::Seconds(1);

// Synchronous proxy to a blink::mojom::PressureService.
class PressureServiceImplSync {
Expand Down Expand Up @@ -190,7 +190,7 @@ TEST_F(PressureServiceImplTest, BindObserver) {
observer.BindNewPipeAndPassRemote()),
blink::mojom::PressureStatus::kOk);

const base::Time time = base::Time::Now() + kRateLimit;
const base::Time time = base::Time::Now();
PressureUpdate update(PressureState::kNominal, {PressureFactor::kThermal},
time);
pressure_manager_overrider_->UpdateClients(update);
Expand All @@ -205,7 +205,7 @@ TEST_F(PressureServiceImplTest, UpdatePressureFactors) {
observer.BindNewPipeAndPassRemote()),
blink::mojom::PressureStatus::kOk);

const base::Time time = base::Time::Now() + kRateLimit;
const base::Time time = base::Time::Now();
PressureUpdate update1(PressureState::kNominal,
{PressureFactor::kPowerSupply}, time);

Expand All @@ -218,48 +218,24 @@ TEST_F(PressureServiceImplTest, UpdatePressureFactors) {
PressureUpdate update2(
PressureState::kCritical,
{PressureFactor::kThermal, PressureFactor::kPowerSupply},
time + kRateLimit * 2.5);
time + kSampleInterval);
pressure_manager_overrider_->UpdateClients(update2);
observer.WaitForUpdate();
ASSERT_EQ(observer.updates().size(), 1u);
EXPECT_EQ(observer.updates()[0], update2);
observer.updates().clear();

PressureUpdate update3(PressureState::kCritical, {PressureFactor::kThermal},
time + kRateLimit * 3.5);
time + kSampleInterval * 2);
pressure_manager_overrider_->UpdateClients(update3);
observer.WaitForUpdate();
ASSERT_EQ(observer.updates().size(), 1u);
EXPECT_EQ(observer.updates()[0], update3);
observer.updates().clear();
}

TEST_F(PressureServiceImplTest, UpdateRateLimiting) {
FakePressureObserver observer;
ASSERT_EQ(pressure_service_impl_sync_->BindObserver(
observer.BindNewPipeAndPassRemote()),
blink::mojom::PressureStatus::kOk);

const base::Time time = base::Time::Now();
PressureUpdate update1(PressureState::kNominal, {PressureFactor::kThermal},
time + kRateLimit);
pressure_manager_overrider_->UpdateClients(update1);
observer.WaitForUpdate();
observer.updates().clear();

// The first update should be blocked due to rate-limiting.
PressureUpdate update2(PressureState::kCritical, {PressureFactor::kThermal},
time + kRateLimit * 1.5);
pressure_manager_overrider_->UpdateClients(update2);
PressureUpdate update3(PressureState::kFair, {PressureFactor::kThermal},
time + kRateLimit * 2);
pressure_manager_overrider_->UpdateClients(update3);
observer.WaitForUpdate();

ASSERT_EQ(observer.updates().size(), 1u);
EXPECT_EQ(observer.updates()[0], update3);
}

// TODO(crbug.com/1385588): Remove this when "passes privacy test" steps are
// implemented.
TEST_F(PressureServiceImplTest, NoVisibility) {
FakePressureObserver observer;
ASSERT_EQ(pressure_service_impl_sync_->BindObserver(
Expand All @@ -271,20 +247,18 @@ TEST_F(PressureServiceImplTest, NoVisibility) {
test_rvh()->SimulateWasHidden();

// The first two updates should be blocked due to invisibility.
PressureUpdate update1(PressureState::kNominal, {}, time + kRateLimit);
PressureUpdate update1(PressureState::kNominal, {}, time);
pressure_manager_overrider_->UpdateClients(update1);
PressureUpdate update2(PressureState::kCritical, {PressureFactor::kThermal},
time + kRateLimit * 2);
time + kSampleInterval);
pressure_manager_overrider_->UpdateClients(update2);
task_environment()->RunUntilIdle();

test_rvh()->SimulateWasShown();

// The third update should be dispatched. It should not be rate-limited by the
// time proximity to the second update, because the second update is not
// dispatched.
// The third update should be dispatched.
PressureUpdate update3(PressureState::kFair, {PressureFactor::kThermal},
time + kRateLimit * 2.5);
time + kSampleInterval * 2);
pressure_manager_overrider_->UpdateClients(update3);
observer.WaitForUpdate();

Expand Down

0 comments on commit 00065de

Please sign in to comment.