Commit 2f4eaabf authored by Xing Liu's avatar Xing Liu Committed by Commit Bot

Notification scheduler: Replace std::deque in impression_types.

This CL replaces std::deque with base::circular_deque. This can reduce
the APK size by 160 bytes.

Notice there are some side effects for base::circular_deque, like can't
dereference iterator while modifying the containers. Also std::sort on
base::circular_deque is taking +500 bytes increase in APK size, which
makes this replacement less efficient.

Bug: 972191
Change-Id: I25f4ae780b0f8be5a970ced561c461a68f1c93fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1759228Reviewed-by: default avatarHesen Zhang <hesen@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688248}
parent e1e12c69
......@@ -82,6 +82,7 @@ source_set("unit_tests") {
"icon_store_unittest.cc",
"impression_history_tracker_unittest.cc",
"impression_store_unittest.cc",
"impression_types_unittest.cc",
"init_aware_scheduler_unittest.cc",
"notification_scheduler_unittest.cc",
"notification_store_unittest.cc",
......
......@@ -202,20 +202,18 @@ void ImpressionHistoryTrackerImpl::AnalyzeImpressionHistory(
base::circular_deque<Impression*> dismisses;
base::Time now = clock_->Now();
// Prune out expired impression.
while (!client_state->impressions.empty() &&
now - client_state->impressions.front().create_time >
config_.impression_expiration) {
impression_map_.erase(client_state->impressions.front().guid);
client_state->impressions.pop_front();
SetNeedsUpdate(client_state->type, true);
}
for (auto it = client_state->impressions.begin();
it != client_state->impressions.end();) {
it != client_state->impressions.end(); ++it) {
auto* impression = &*it;
// Prune out expired impression.
if (now - impression->create_time > config_.impression_expiration) {
impression_map_.erase(impression->guid);
client_state->impressions.erase(it++);
SetNeedsUpdate(client_state->type, true);
continue;
} else {
++it;
}
switch (impression->feedback) {
case UserFeedback::kDismiss:
dismisses.emplace_back(impression);
......
......@@ -48,9 +48,16 @@ ClientState::ClientState(const ClientState& other) = default;
ClientState::~ClientState() = default;
bool ClientState::operator==(const ClientState& other) const {
if (impressions.size() != other.impressions.size())
return false;
for (size_t i = 0; i < impressions.size(); ++i) {
if (!(impressions[i] == other.impressions[i]))
return false;
}
return type == other.type &&
current_max_daily_show == other.current_max_daily_show &&
impressions == other.impressions &&
suppression_info == other.suppression_info;
}
......
......@@ -5,10 +5,10 @@
#ifndef CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_IMPRESSION_TYPES_H_
#define CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_IMPRESSION_TYPES_H_
#include <deque>
#include <map>
#include <string>
#include "base/containers/circular_deque.h"
#include "base/optional.h"
#include "base/time/time.h"
#include "chrome/browser/notifications/scheduler/public/notification_scheduler_types.h"
......@@ -97,7 +97,7 @@ struct SuppressionInfo {
// to the user and the history of user interactions to a particular notification
// client.
struct ClientState {
using Impressions = std::deque<Impression>;
using Impressions = base::circular_deque<Impression>;
ClientState();
explicit ClientState(const ClientState& other);
~ClientState();
......
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chrome/browser/notifications/scheduler/internal/impression_types.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace notifications {
namespace {
// Verifies equal operator of ClientState.
TEST(ImpressionTypesTest, ClientStateEqual) {
ClientState client_state0, client_state1;
EXPECT_EQ(client_state0, client_state1);
client_state0.impressions.emplace_back(Impression());
EXPECT_FALSE(client_state0 == client_state1);
client_state1.impressions.emplace_back(Impression());
EXPECT_EQ(client_state0, client_state1);
client_state0.impressions.front().guid = "guid";
EXPECT_FALSE(client_state0 == client_state1);
}
} // namespace
} // namespace notifications
......@@ -8,6 +8,7 @@
#include <utility>
#include <vector>
#include "base/containers/circular_deque.h"
#include "base/task/post_task.h"
#include "base/threading/thread_restrictions.h"
#include "chrome/browser/notifications/scheduler/internal/impression_types.h"
......@@ -16,11 +17,12 @@
namespace notifications {
namespace {
using FirstAndLastIters = std::pair<std::deque<Impression>::const_iterator,
std::deque<Impression>::const_iterator>;
using FirstAndLastIters =
std::pair<base::circular_deque<Impression>::const_iterator,
base::circular_deque<Impression>::const_iterator>;
base::Optional<FirstAndLastIters> FindFirstAndLastNotificationShownToday(
const std::deque<Impression>& impressions,
const base::circular_deque<Impression>& impressions,
const base::Time& now,
const base::Time& beginning_of_today) {
if (impressions.empty() || impressions.cbegin()->create_time > now ||
......
......@@ -5,7 +5,6 @@
#ifndef CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_SCHEDULER_UTILS_H_
#define CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_SCHEDULER_UTILS_H_
#include <deque>
#include <map>
#include <memory>
#include <string>
......
......@@ -6,6 +6,7 @@
#include <algorithm>
#include "base/containers/circular_deque.h"
#include "base/guid.h"
#include "base/test/scoped_task_environment.h"
#include "chrome/browser/notifications/scheduler/internal/impression_types.h"
......@@ -31,9 +32,9 @@ class SchedulerUtilsTest : public testing::Test {
}
void CreateFakeImpressions(std::vector<base::Time> times,
std::deque<Impression>& impressions) {
base::circular_deque<Impression>& impressions) {
impressions.clear();
for (auto time : times) {
for (const auto& time : times) {
impressions.emplace_back(SchedulerClientType::kTest1,
base::GenerateGUID(), time);
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment