0

Add a Finch-tunable parameter for the loader's navigation chunk size

See [1] for a related CL. This CL adds a parameter to [1] to control
the NavigationBodyLoader's chunk size, for experimentation. See [2]
for the eventual goal of increasing that size from 512kB to ~2MB or
more. But this CL will allow experimentation first, to ensure no
memory regressions, and find a good final value.

Without any changes via Finch, this CL should have no effect on
behavior.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/3027323
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2983494

Bug: 1041006
Change-Id: Ic9db4516262ee1ecce455806d20aae28e0432a54
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3027705
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#902692}
This commit is contained in:
Mason Freed
2021-07-16 21:57:01 +00:00
committed by Chromium LUCI CQ
parent 1f4d855e68
commit ae4ad40514
8 changed files with 46 additions and 40 deletions

@ -219,7 +219,7 @@ const base::Feature kFtpProtocol{"FtpProtocol",
const base::Feature kSCTAuditingRetryAndPersistReports{
"SCTAuditingRetryAndPersistReports", base::FEATURE_DISABLED_BY_DEFAULT};
// This feature will be used for tuning several loading-related data pipe
// This feature is used for tuning several loading-related data pipe
// parameters. See crbug.com/1041006.
const base::Feature kLoaderDataPipeTuningFeature{
"LoaderDataPipeTuning", base::FEATURE_DISABLED_BY_DEFAULT};
@ -230,6 +230,15 @@ static constexpr uint32_t kDataPipeDefaultAllocationSize = 512 * 1024;
constexpr base::FeatureParam<int> kDataPipeAllocationSize{
&kLoaderDataPipeTuningFeature, "allocation_size_bytes",
base::saturated_cast<int>(kDataPipeDefaultAllocationSize)};
// The maximal number of bytes consumed in a loading task. When there are more
// bytes in the data pipe, they will be consumed in following tasks. Setting too
// small of a number will generate many tasks but setting a too large of a
// number will lead to thread janks.
static constexpr uint32_t kMaxNumConsumedBytesInTask = 64 * 1024;
constexpr base::FeatureParam<int> kLoaderChunkSize{
&kLoaderDataPipeTuningFeature, "loader_chunk_size",
base::saturated_cast<int>(kMaxNumConsumedBytesInTask)};
} // namespace
// static
@ -237,5 +246,10 @@ uint32_t GetDataPipeDefaultAllocationSize() {
return base::saturated_cast<uint32_t>(kDataPipeAllocationSize.Get());
}
// static
uint32_t GetLoaderChunkSize() {
return base::saturated_cast<uint32_t>(kLoaderChunkSize.Get());
}
} // namespace features
} // namespace network

@ -91,6 +91,9 @@ extern const base::Feature kLoaderDataPipeTuningFeature;
COMPONENT_EXPORT(NETWORK_CPP)
extern uint32_t GetDataPipeDefaultAllocationSize();
COMPONENT_EXPORT(NETWORK_CPP)
extern uint32_t GetLoaderChunkSize();
} // namespace features
} // namespace network

@ -10,6 +10,7 @@
#include "base/auto_reset.h"
#include "base/metrics/histogram_macros.h"
#include "base/trace_event/trace_event.h"
#include "services/network/public/cpp/features.h"
#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/mojom/frame/back_forward_cache_controller.mojom-blink.h"
#include "third_party/blink/renderer/platform/back_forward_cache_utils.h"
@ -17,12 +18,11 @@
#include "third_party/blink/renderer/platform/loader/fetch/fetch_context.h"
#include "third_party/blink/renderer/platform/loader/fetch/resource.h"
#include "third_party/blink/renderer/platform/loader/fetch/resource_loader.h"
#include "third_party/blink/renderer/platform/loader/fetch/url_loader/navigation_body_loader.h"
#include "third_party/blink/renderer/platform/wtf/shared_buffer.h"
namespace blink {
constexpr size_t ResponseBodyLoader::kMaxNumConsumedBytesInTask;
constexpr size_t kDefaultMaxBufferedBodyBytesPerRequest = 100 * 1000;
class ResponseBodyLoader::DelegatingBytesConsumer final
@ -567,7 +567,8 @@ void ResponseBodyLoader::OnStateChange() {
size_t num_bytes_consumed = 0;
while (!aborted_ && (!IsSuspended() || IsSuspendedForBackForwardCache())) {
if (kMaxNumConsumedBytesInTask == num_bytes_consumed) {
uint32_t chunk_size = network::features::GetLoaderChunkSize();
if (chunk_size == num_bytes_consumed) {
// We've already consumed many bytes in this task. Defer the remaining
// to the next task.
task_runner_->PostTask(FROM_HERE,
@ -579,8 +580,8 @@ void ResponseBodyLoader::OnStateChange() {
if (!IsSuspended() && body_buffer_ && !body_buffer_->IsEmpty()) {
// We need to empty |body_buffer_| first before reading more from
// |bytes_consumer_|.
num_bytes_consumed += body_buffer_->DispatchChunk(
kMaxNumConsumedBytesInTask - num_bytes_consumed);
num_bytes_consumed +=
body_buffer_->DispatchChunk(chunk_size - num_bytes_consumed);
continue;
}
@ -595,8 +596,7 @@ void ResponseBodyLoader::OnStateChange() {
base::AutoReset<bool> auto_reset_for_in_two_phase_read(
&in_two_phase_read_, true);
available =
std::min(available, kMaxNumConsumedBytesInTask - num_bytes_consumed);
available = std::min(available, chunk_size - num_bytes_consumed);
if (IsSuspendedForBackForwardCache()) {
// Save the read data into |body_buffer_| instead.
DidBufferLoadWhileInBackForwardCache(available);

@ -121,13 +121,6 @@ class PLATFORM_EXPORT ResponseBodyLoader final
void Trace(Visitor*) const override;
// The maximal number of bytes consumed in a task. When there are more bytes
// in the data pipe, they will be consumed in following tasks. Setting a too
// small number will generate ton of tasks but setting a too large number will
// lead to thread janks. Also, some clients cannot handle too large chunks
// (512k for example).
static constexpr size_t kMaxNumConsumedBytesInTask = 64 * 1024;
private:
class Buffer;
class DelegatingBytesConsumer;

@ -8,6 +8,7 @@
#include <string>
#include <utility>
#include "base/test/scoped_feature_list.h"
#include "services/network/public/cpp/features.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/platform/web_runtime_features.h"
@ -44,6 +45,16 @@ class ResponseBodyLoaderTest : public testing::Test {
using Command = ReplayingBytesConsumer::Command;
using PublicState = BytesConsumer::PublicState;
using Result = BytesConsumer::Result;
static constexpr uint32_t kMaxNumConsumedBytesInTaskForTesting = 512 * 1024;
ResponseBodyLoaderTest() {
base::FieldTrialParams params;
params["loader_chunk_size"] =
base::NumberToString(kMaxNumConsumedBytesInTaskForTesting);
scoped_feature_list_.InitAndEnableFeatureWithParameters(
network::features::kLoaderDataPipeTuningFeature, params);
}
class TestClient final : public GarbageCollected<TestClient>,
public ResponseBodyLoaderClient {
public:
@ -160,6 +171,9 @@ class ResponseBodyLoaderTest : public testing::Test {
bytes_consumer, client, task_runner,
MakeGarbageCollected<TestBackForwardCacheLoaderHelper>());
}
private:
base::test::ScopedFeatureList scoped_feature_list_;
};
class ResponseBodyLoaderDrainedBytesConsumerNotificationOutOfOnStateChangeTest
@ -346,7 +360,7 @@ TEST_F(ResponseBodyLoaderTest, Suspend) {
TEST_F(ResponseBodyLoaderTest, ReadTooBigBuffer) {
auto task_runner = base::MakeRefCounted<scheduler::FakeTaskRunner>();
auto* consumer = MakeGarbageCollected<ReplayingBytesConsumer>(task_runner);
constexpr auto kMax = ResponseBodyLoader::kMaxNumConsumedBytesInTask;
constexpr auto kMax = kMaxNumConsumedBytesInTaskForTesting;
consumer->Add(Command(Command::kData, std::string(kMax - 1, 'a').data()));
consumer->Add(Command(Command::kData, std::string(2, 'b').data()));

@ -6,9 +6,12 @@
#include "base/bind.h"
#include "base/macros.h"
#include "base/metrics/field_trial_params.h"
#include "base/metrics/histogram_functions.h"
#include "base/numerics/safe_conversions.h"
#include "base/strings/strcat.h"
#include "base/trace_event/trace_event.h"
#include "services/network/public/cpp/features.h"
#include "services/network/public/cpp/url_loader_completion_status.h"
#include "services/network/public/mojom/early_hints.mojom.h"
#include "services/network/public/mojom/url_response_head.mojom.h"
@ -22,9 +25,6 @@
namespace blink {
// static
constexpr uint32_t NavigationBodyLoader::kMaxNumConsumedBytesInTask;
NavigationBodyLoader::NavigationBodyLoader(
const KURL& original_url,
network::mojom::URLResponseHeadPtr response_head,
@ -233,9 +233,9 @@ void NavigationBodyLoader::ReadFromDataPipe() {
NotifyCompletionIfAppropriate();
return;
}
DCHECK_LE(num_bytes_consumed, kMaxNumConsumedBytesInTask);
available =
std::min(available, kMaxNumConsumedBytesInTask - num_bytes_consumed);
uint32_t chunk_size = network::features::GetLoaderChunkSize();
DCHECK_LE(num_bytes_consumed, chunk_size);
available = std::min(available, chunk_size - num_bytes_consumed);
if (available == 0) {
// We've already consumed many bytes in this task. Defer the remaining
// to the next task.
@ -374,5 +374,4 @@ void WebNavigationBodyLoader::FillNavigationParamsResponseAndBodyLoader(
std::move(resource_load_info_notifier_wrapper), is_main_frame));
}
}
} // namespace blink

@ -79,13 +79,6 @@ class NavigationBodyLoader : public WebNavigationBodyLoader,
// NotifyCompletionIfAppropriate
// notify client about completion
// The maximal number of bytes consumed in a task. When there are more bytes
// in the data pipe, they will be consumed in following tasks. Setting a too
// small number will generate ton of tasks but setting a too large number will
// lead to thread janks. Also, some clients cannot handle too large chunks
// (512k for example).
static constexpr uint32_t kMaxNumConsumedBytesInTask = 64 * 1024;
// WebNavigationBodyLoader implementation.
void SetDefersLoading(WebLoaderFreezeMode mode) override;
void StartLoadingBody(WebNavigationBodyLoader::Client* client,

@ -373,13 +373,6 @@ class WebURLLoader::Context : public WebRequestPeer {
std::unique_ptr<WebResourceRequestSender> resource_request_sender);
private:
// The maximal number of bytes consumed in a task. When there are more bytes
// in the data pipe, they will be consumed in following tasks. Setting a too
// small number will generate ton of tasks but setting a too large number will
// lead to thread janks. Also, some clients cannot handle too large chunks
// (512k for example).
static constexpr uint32_t kMaxNumConsumedBytesInTask = 64 * 1024;
~Context() override;
// Called when the body data stream is detached from the reader side.
@ -436,9 +429,6 @@ class WebURLLoader::Context : public WebRequestPeer {
// WebURLLoader::Context -------------------------------------------------------
// static
constexpr uint32_t WebURLLoader::Context::kMaxNumConsumedBytesInTask;
WebURLLoader::Context::Context(
WebURLLoader* loader,
const WebVector<WebString>& cors_exempt_header_list,