Skip to content

GitLab

T
tangled
Commit 5c2da042 authored by Yafei Duan's avatar Yafei Duan Committed by Commit Bot
Browse files
Options

[Offline Pages] Improving ArchiveManager and storage stats. 

In this CL:
- Removed unused methods from ArchiveManager.
- Reporting more storage stats from ArchiveManager, for multiple archive
  directories.
- Fixed multiple tests related.

There will be a followup change based on this one for reporting storage
related UMAs from OfflinePageModelTaskified.

Bug: NONE
Change-Id: I8f9d2837b4c47a1f88fd380c86f63d0f5340c6e0
Reviewed-on: https://chromium-review.googlesource.com/967561
Commit-Queue: Yafei Duan <romax@chromium.org>
Reviewed-by: default avatarPeter Williamson <petewil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544993}
parent bc090adf
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 154 additions and 341 deletions
components/offline_pages/core/archive_manager.cc
View file @ 5c2da042
......@@ -13,15 +13,17 @@
#include "base/memory/ref_counted.h"
#include "base/metrics/histogram_macros.h"
#include "base/sequenced_task_runner.h"
#include "base/strings/utf_string_conversions.h"
#include "base/sys_info.h"
#include "base/threading/thread_task_runner_handle.h"
#include "build/build_config.h"
namespace offline_pages {
namespace {
using StorageStatsCallback =
base::Callback<void(const ArchiveManager::StorageStats& storage_stats)>;
base::OnceCallback<void(const ArchiveManager::StorageStats& storage_stats)>;
void EnsureArchivesDirCreatedImpl(const base::FilePath& archives_dir,
bool is_temp) {
......@@ -43,63 +45,47 @@ void EnsureArchivesDirCreatedImpl(const base::FilePath& archives_dir,
}
}
void ExistsArchiveImpl(const base::FilePath& file_path,
scoped_refptr<base::SequencedTaskRunner> task_runner,
const base::Callback<void(bool)>& callback) {
task_runner->PostTask(FROM_HERE,
base::Bind(callback, base::PathExists(file_path)));
}
void DeleteArchivesImpl(const std::vector<base::FilePath>& file_paths,
scoped_refptr<base::SequencedTaskRunner> task_runner,
const base::Callback<void(bool)>& callback) {
bool result = false;
for (const auto& file_path : file_paths) {
// Make sure delete happens on the left of || so that it is always executed.
result = base::DeleteFile(file_path, false) || result;
}
task_runner->PostTask(FROM_HERE, base::Bind(callback, result));
}
void GetAllArchivesImpl(
const std::vector<base::FilePath>& archives_dirs,
scoped_refptr<base::SequencedTaskRunner> task_runner,
const base::Callback<void(const std::set<base::FilePath>&)>& callback) {
std::set<base::FilePath> archive_paths;
for (const auto& archives_dir : archives_dirs) {
base::FileEnumerator file_enumerator(archives_dir, false,
base::FileEnumerator::FILES);
for (base::FilePath archive_path = file_enumerator.Next();
!archive_path.empty(); archive_path = file_enumerator.Next()) {
archive_paths.insert(archive_path);
}
}
task_runner->PostTask(FROM_HERE, base::Bind(callback, archive_paths));
}
void GetStorageStatsImpl(const base::FilePath& temporary_archives_dir,
const base::FilePath& persistent_archives_dir,
const base::FilePath& private_archives_dir,
const base::FilePath& public_archives_dir,
scoped_refptr<base::SequencedTaskRunner> task_runner,
const StorageStatsCallback& callback) {
ArchiveManager::StorageStats storage_stats = {0, 0, 0};
StorageStatsCallback callback) {
ArchiveManager::StorageStats storage_stats = {0, 0, 0, 0, 0};
// Getting the free disk space of the volume that contains the temporary
// archives directory. This value will be -1 if the directory is invalid.
// Currently both temporary and persistent archives directories are in the
// Currently both temporary and private archive directories are in the
// internal storage.
// In the future temporary and persistent archives directories may be on
// different volumes, then another field may be added to StorageStats.
storage_stats.free_disk_space =
storage_stats.internal_free_disk_space =
base::SysInfo::AmountOfFreeDiskSpace(temporary_archives_dir);
if (!persistent_archives_dir.empty()) {
storage_stats.private_archives_size =
base::ComputeDirectorySize(persistent_archives_dir);
}
storage_stats.external_free_disk_space =
base::SysInfo::AmountOfFreeDiskSpace(public_archives_dir);
if (!temporary_archives_dir.empty()) {
storage_stats.temporary_archives_size =
base::ComputeDirectorySize(temporary_archives_dir);
}
task_runner->PostTask(FROM_HERE, base::Bind(callback, storage_stats));
if (!private_archives_dir.empty()) {
storage_stats.private_archives_size =
base::ComputeDirectorySize(private_archives_dir);
}
if (!public_archives_dir.empty()) {
base::FileEnumerator file_enumerator(public_archives_dir, false,
base::FileEnumerator::FILES);
while (!file_enumerator.Next().empty()) {
#if defined(OS_WIN)
std::string extension = base::WideToUTF8(
file_enumerator.GetInfo().GetName().FinalExtension());
#else
std::string extension =
file_enumerator.GetInfo().GetName().FinalExtension();
#endif
if (extension == "mhtml" || extension == "mht")
storage_stats.public_archives_size +=
file_enumerator.GetInfo().GetSize();
}
}
task_runner->PostTask(FROM_HERE,
base::BindOnce(std::move(callback), storage_stats));
}
} // namespace
......@@ -119,7 +105,8 @@ ArchiveManager::ArchiveManager(
ArchiveManager::~ArchiveManager() {}
void ArchiveManager::EnsureArchivesDirCreated(const base::Closure& callback) {
void ArchiveManager::EnsureArchivesDirCreated(
base::OnceCallback<void()> callback) {
// The callback will only be invoked once both directories are created.
if (!temporary_archives_dir_.empty()) {
task_runner_->PostTask(
......@@ -130,47 +117,15 @@ void ArchiveManager::EnsureArchivesDirCreated(const base::Closure& callback) {
FROM_HERE,
base::Bind(EnsureArchivesDirCreatedImpl, private_archives_dir_,
false /* is_temp */),
callback);
}
void ArchiveManager::ExistsArchive(const base::FilePath& archive_path,
const base::Callback<void(bool)>& callback) {
task_runner_->PostTask(
FROM_HERE, base::Bind(ExistsArchiveImpl, archive_path,
base::ThreadTaskRunnerHandle::Get(), callback));
}
void ArchiveManager::DeleteArchive(const base::FilePath& archive_path,
const base::Callback<void(bool)>& callback) {
std::vector<base::FilePath> archive_paths = {archive_path};
DeleteMultipleArchives(archive_paths, callback);
std::move(callback));
}
void ArchiveManager::DeleteMultipleArchives(
const std::vector<base::FilePath>& archive_paths,
const base::Callback<void(bool)>& callback) {
void ArchiveManager::GetStorageStats(StorageStatsCallback callback) const {
task_runner_->PostTask(
FROM_HERE, base::Bind(DeleteArchivesImpl, archive_paths,
base::ThreadTaskRunnerHandle::Get(), callback));
}
void ArchiveManager::GetAllArchives(
const base::Callback<void(const std::set<base::FilePath>&)>& callback)
const {
std::vector<base::FilePath> archives_dirs = {private_archives_dir_};
if (!temporary_archives_dir_.empty())
archives_dirs.push_back(temporary_archives_dir_);
task_runner_->PostTask(
FROM_HERE, base::Bind(GetAllArchivesImpl, archives_dirs,
base::ThreadTaskRunnerHandle::Get(), callback));
}
void ArchiveManager::GetStorageStats(
const StorageStatsCallback& callback) const {
task_runner_->PostTask(
FROM_HERE, base::Bind(GetStorageStatsImpl, temporary_archives_dir_,
private_archives_dir_,
base::ThreadTaskRunnerHandle::Get(), callback));
FROM_HERE,
base::BindOnce(GetStorageStatsImpl, temporary_archives_dir_,
private_archives_dir_, public_archives_dir_,
base::ThreadTaskRunnerHandle::Get(), std::move(callback)));
}
const base::FilePath& ArchiveManager::GetTemporaryArchivesDir() const {
......
components/offline_pages/core/archive_manager.h
View file @ 5c2da042
......@@ -5,9 +5,6 @@
#ifndef COMPONENTS_OFFLINE_PAGES_CORE_ARCHIVE_MANAGER_H_
#define COMPONENTS_OFFLINE_PAGES_CORE_ARCHIVE_MANAGER_H_
#include <set>
#include <vector>
#include "base/callback_forward.h"
#include "base/files/file_path.h"
#include "base/memory/ref_counted.h"
......@@ -22,23 +19,30 @@ namespace offline_pages {
// different archive directories based on their lifetime types (persistent or
// temporary).
// All tasks are performed using |task_runner_|.
// TODO(romax): When P2P sharing comes and pages saved in internal storage get
// moved out to external storage, make sure nothing breaks (and it's necessary
// to check if both internal/external are on the volume, in case any logic
// breaks.)
class ArchiveManager {
public:
// Used by metrics collection and clearing storage of temporary pages. The
// |free_disk_space| will be the free disk space of the volume that contains
// temporary archives directory. Another field may be added if the free disk
// space of persistent archives is needed in future.
// Used by metrics collection and clearing storage of temporary pages.
// - |internal_free_disk_space| is the free space of the volume which
// contains temporary and private archive directories.
// - |external_free_disk_space| is the free space of the volume which contains
// public archives directory, in most cases it should be Download directory
// in /sdcard/.
// - |{temporary/private}_archives_size| is the size of the directory. Since
// these are inside app directory and are fully controlled, it's unnecessary
// to calculate the size by iterating and counting.
// - |public_archives_size| will enumerate all mhtml files in the public
// directory and records the total file size. This will include mhtml files
// shared into the public directory through other approaches.
struct StorageStats {
int64_t total_archives_size() const {
return temporary_archives_size + private_archives_size;
return temporary_archives_size + private_archives_size +
public_archives_size;
}
int64_t free_disk_space;
int64_t internal_free_disk_space;
int64_t external_free_disk_space;
int64_t temporary_archives_size;
int64_t private_archives_size;
int64_t public_archives_size;
};
ArchiveManager(const base::FilePath& temporary_archives_dir,
......@@ -48,35 +52,12 @@ class ArchiveManager {
virtual ~ArchiveManager();
// Creates archives directory if one does not exist yet;
virtual void EnsureArchivesDirCreated(const base::Closure& callback);
// Checks whether an archive with specified |archive_path| exists.
virtual void ExistsArchive(const base::FilePath& archive_path,
const base::Callback<void(bool)>& callback);
// Deletes an archive with specified |archive_path|.
// It is considered successful to attempt to delete a file that does not
// exist.
virtual void DeleteArchive(const base::FilePath& archive_path,
const base::Callback<void(bool)>& callback);
// Deletes multiple archives that are specified in |archive_paths|.
// It is considered successful to attempt to delete a file that does not
// exist.
virtual void DeleteMultipleArchives(
const std::vector<base::FilePath>& archive_paths,
const base::Callback<void(bool)>& callback);
// Lists all archive files in both temporary and persistent archive
// directories.
virtual void GetAllArchives(
const base::Callback<void(const std::set<base::FilePath>&)>& callback)
const;
virtual void EnsureArchivesDirCreated(base::OnceCallback<void()> callback);
// Gets stats about archive storage, i.e. sizes of temporary and persistent
// archive files and free disk space.
// Gets stats about archive storage, i.e. sizes of all archive directories
// and free disk spaces.
virtual void GetStorageStats(
const base::Callback<void(const StorageStats& storage_sizes)>& callback)
base::OnceCallback<void(const StorageStats& storage_sizes)> callback)
const;
// Gets the archive directories.
......@@ -85,6 +66,7 @@ class ArchiveManager {
const base::FilePath& GetPublicArchivesDir() const;
protected:
// Used for testing.
ArchiveManager();
private:
......
components/offline_pages/core/model/clear_storage_task.cc
View file @ 5c2da042
......@@ -110,10 +110,10 @@ std::unique_ptr<std::vector<PageInfo>> GetPageInfosToClear(
// just expired pages to make the storage usage below the threshold.
bool quota_based_clearing =
stats.temporary_archives_size >=
(stats.temporary_archives_size + stats.free_disk_space) *
(stats.temporary_archives_size + stats.internal_free_disk_space) *
kOfflinePageStorageLimit;
int64_t max_allowed_size =
(stats.temporary_archives_size + stats.free_disk_space) *
(stats.temporary_archives_size + stats.internal_free_disk_space) *
kOfflinePageStorageClearThreshold;
// Initialize the counting map with 0s.
......
components/offline_pages/core/model/clear_storage_task_unittest.cc
View file @ 5c2da042
......@@ -8,6 +8,8 @@
#include "base/bind.h"
#include "base/files/file_enumerator.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/test/histogram_tester.h"
#include "base/test/simple_test_clock.h"
#include "base/time/time.h"
......@@ -40,21 +42,29 @@ struct PageSettings {
class TestArchiveManager : public ArchiveManager {
public:
explicit TestArchiveManager(OfflinePageMetadataStoreTestUtil* store_test_util)
: free_space_(kFreeSpaceNormal), store_test_util_(store_test_util) {}
explicit TestArchiveManager(const base::FilePath& temp_archive_dir)
: free_space_(kFreeSpaceNormal),
temporary_archive_dir_(temp_archive_dir) {}
void GetStorageStats(
const base::Callback<void(const StorageStats& storage_stats)>& callback)
base::OnceCallback<void(const StorageStats& storage_stats)> callback)
const override {
callback.Run(
{free_space_, store_test_util_->GetPageCount() * kTestFileSize});
StorageStats stats;
stats.internal_free_disk_space = free_space_;
base::FileEnumerator file_enumerator(temporary_archive_dir_, false,
base::FileEnumerator::FILES);
int temp_file_count = 0;
while (!file_enumerator.Next().empty())
temp_file_count++;
stats.temporary_archives_size = temp_file_count * kTestFileSize;
std::move(callback).Run(stats);
}
void SetFreeSpace(int64_t free_space) { free_space_ = free_space; }
private:
int64_t free_space_;
OfflinePageMetadataStoreTestUtil* store_test_util_;
base::FilePath temporary_archive_dir_;
};
} // namespace
......@@ -117,7 +127,7 @@ void ClearStorageTaskTest::Initialize(
// Adding pages based on |page_settings|.
for (const auto& setting : page_settings)
AddPages(setting);
archive_manager_.reset(new TestArchiveManager(store_test_util()));
archive_manager_.reset(new TestArchiveManager(TemporaryDir()));
}
void ClearStorageTaskTest::OnClearStorageDone(size_t cleared_page_count,
......@@ -133,8 +143,18 @@ void ClearStorageTaskTest::AddPages(const PageSettings& setting) {
// this choice allows for easier testing of the TimeSinceCreation metric. This
// way we can work directly with the times used to advance the testing clock
// during each test.
// Make sure no persistent pages are marked as expired.
if (!policy_controller()->IsRemovedOnCacheReset(setting.name_space))
ASSERT_FALSE(setting.expired_page_count);
generator()->SetCreationTime(clock()->Now());
generator()->SetNamespace(setting.name_space);
if (policy_controller()->IsRemovedOnCacheReset(setting.name_space)) {
generator()->SetArchiveDirectory(TemporaryDir());
} else {
generator()->SetArchiveDirectory(PrivateDir());
}
for (int i = 0; i < setting.fresh_page_count; ++i) {
generator()->SetLastAccessTime(clock_.Now());
......@@ -207,7 +227,8 @@ TEST_F(ClearStorageTaskTest, TryClearPersistentPages) {
EXPECT_EQ(1, total_cleared_times());
EXPECT_EQ(ClearStorageResult::UNNECESSARY, last_clear_storage_result());
EXPECT_EQ(20LL, store_test_util()->GetPageCount());
EXPECT_EQ(20UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
EXPECT_EQ(0UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
EXPECT_EQ(20UL, test_utils::GetFileCountInDirectory(PrivateDir()));
histogram_tester()->ExpectTotalCount(
"OfflinePages.ClearTemporaryPages.TimeSinceCreation", 0);
}
......@@ -226,13 +247,14 @@ TEST_F(ClearStorageTaskTest, TryClearPersistentPagesWithStoragePressure) {
EXPECT_EQ(1, total_cleared_times());
EXPECT_EQ(ClearStorageResult::UNNECESSARY, last_clear_storage_result());
EXPECT_EQ(20LL, store_test_util()->GetPageCount());
EXPECT_EQ(20UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
EXPECT_EQ(0UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
EXPECT_EQ(20UL, test_utils::GetFileCountInDirectory(PrivateDir()));
}
TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
// Initializing with 20 unexpired and 0 expired pages in bookmark namespace,
// 30 unexpired and 1 expired pages in last_n namespace, and 40 persistent
// pages in download namespace. Free space on the disk is 200MB.
// pages in download namespace.
Initialize({{kBookmarkNamespace, 20, 0},
{kLastNNamespace, 30, 1},
{kDownloadNamespace, 40, 0}});
......@@ -258,12 +280,12 @@ TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
RunClearStorageTask(clock()->Now());
// There's only 1 expired pages, so it will be cleared. There will be (30 +
// 20 + 40) pages remaining.
// 20 + 40) pages remaining, 40 of them are persistent pages.
EXPECT_EQ(1UL, last_cleared_page_count());
EXPECT_EQ(1, total_cleared_times());
EXPECT_EQ(ClearStorageResult::SUCCESS, last_clear_storage_result());
EXPECT_EQ(90LL, store_test_util()->GetPageCount());
EXPECT_EQ(90UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
EXPECT_EQ(50UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
histogram_tester()->ExpectUniqueSample(
"OfflinePages.ClearTemporaryPages.TimeSinceCreation", 30, 1);
......@@ -273,12 +295,12 @@ TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
RunClearStorageTask(clock()->Now());
// All pages in bookmark namespace should be cleared. And only 70 pages
// remaining after the clearing.
// remaining after the clearing, 40 of them are persistent pages.
EXPECT_EQ(20UL, last_cleared_page_count());
EXPECT_EQ(2, total_cleared_times());
EXPECT_EQ(ClearStorageResult::SUCCESS, last_clear_storage_result());
EXPECT_EQ(70LL, store_test_util()->GetPageCount());
EXPECT_EQ(70UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
EXPECT_EQ(30UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
histogram_tester()->ExpectTotalCount(
"OfflinePages.ClearTemporaryPages.TimeSinceCreation", 21);
histogram_tester()->ExpectBucketCount(
......@@ -295,31 +317,32 @@ TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
EXPECT_EQ(3, total_cleared_times());
EXPECT_EQ(ClearStorageResult::UNNECESSARY, last_clear_storage_result());
EXPECT_EQ(70LL, store_test_util()->GetPageCount());
EXPECT_EQ(70UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
EXPECT_EQ(30UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
histogram_tester()->ExpectTotalCount(
"OfflinePages.ClearTemporaryPages.TimeSinceCreation", 21);
// Adding more fresh pages in last_n namespace to make storage usage exceed
// limit, so even if only 5 minutes passed from last clearing, this will still
// clear some pages.
// Free storage space is 200MB and all pages take 270 * 0.5MB = 135MB (while
// temporary pages takes 230 * 0.5MB = 115MB), which is over (135MB + 200MB)
// * 0.3 = 100.5MB. In order to bring the storage usage down to (135MB +
// 200MB) * 0.1 = 33.5MB, (115MB - 33.5MB) needs to be released, which is 163
// temporary pages to be cleared.
AddPages({kLastNNamespace, 200, 0});
// Free storage space is 200MB and all temporary pages take 270 * 0.5MB =
// 135MB, which is over (135MB + 200MB) * 0.3 = 100.5MB.
// In order to bring the storage usage down to (135MB + 200MB) * 0.1 = 33.5MB,
// (135MB - 33.5MB) needs to be released, which means 203 temporary pages need
// to be cleared.
AddPages({kLastNNamespace, 240, 0});
SetFreeSpace(200 * (1 << 20));
clock()->Advance(base::TimeDelta::FromMinutes(5));
RunClearStorageTask(clock()->Now());
// There should be 107 pages remaining after the clearing.
EXPECT_EQ(163UL, last_cleared_page_count());
// There should be 107 pages remaining after the clearing (including 40
// persistent pages).
EXPECT_EQ(203UL, last_cleared_page_count());
EXPECT_EQ(4, total_cleared_times());
EXPECT_EQ(ClearStorageResult::SUCCESS, last_clear_storage_result());
EXPECT_EQ(107LL, store_test_util()->GetPageCount());
EXPECT_EQ(107UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
EXPECT_EQ(67UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
histogram_tester()->ExpectTotalCount(
"OfflinePages.ClearTemporaryPages.TimeSinceCreation", 184);
"OfflinePages.ClearTemporaryPages.TimeSinceCreation", 224);
// The 30 original ones last_n pages are cleared (and they fall into the same
// bucket as the 20 from bookmarks)...
histogram_tester()->ExpectBucketCount(
......@@ -327,7 +350,7 @@ TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
30 + bookmark_policy.expiration_period.InMinutes() + 5, 20 + 30);
// ... As well as 133 from this latest round.
histogram_tester()->ExpectBucketCount(
"OfflinePages.ClearTemporaryPages.TimeSinceCreation", 5, 133);
"OfflinePages.ClearTemporaryPages.TimeSinceCreation", 5, 173);
// Advance the clock by 300 days, in order to expire all temporary pages. Only
// 67 temporary pages are left from the last clearing.
......@@ -339,9 +362,9 @@ TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
EXPECT_EQ(5, total_cleared_times());
EXPECT_EQ(ClearStorageResult::SUCCESS, last_clear_storage_result());
EXPECT_EQ(40LL, store_test_util()->GetPageCount());
EXPECT_EQ(40UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
EXPECT_EQ(0UL, test_utils::GetFileCountInDirectory(TemporaryDir()));
histogram_tester()->ExpectTotalCount(
"OfflinePages.ClearTemporaryPages.TimeSinceCreation", 251);
"OfflinePages.ClearTemporaryPages.TimeSinceCreation", 291);
histogram_tester()->ExpectBucketCount(
"OfflinePages.ClearTemporaryPages.TimeSinceCreation",
base::TimeDelta::FromDays(300).InMinutes() + 5, 67);
......
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