New issue
Advanced search Search tips

Issue 787704 link

Starred by 0 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 794651



Sign in to add a comment

Consider using std::unique_ptr for blink::ResourceRequest and blink::ResourceResponse

Project Member Reported by tyoshino@chromium.org, Nov 22 2017

Issue description

We guess there are unnecessary copies happening causing some performance issues.

Consider making them again std::unique_ptr wrapped (originally they were OwnPtr wrapped a while ago).

 
Cc: toyoshim@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0d92c5d1765633b60e397df001cad5894935467

commit d0d92c5d1765633b60e397df001cad5894935467
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Tue Nov 28 08:12:29 2017

Move redirect ResourceRequest creation code to ResourceRequest

ResourceLoader had a code that constructs a new ResourceRequest
for a redirect, and setup properties based on the last ResourceRequest.

This code will be broken easily when people add new items to
ResourceRequest because it's hard to notice that we also need to
copy the new items even in ResourceLoader for redirection.

This patch moves the creation code to ResourceRequest so that
people who are going to change ResourceRequest can easily notice
the requirement.

Bug: 736308, 787704
Change-Id: I605b4c20f14b02fc5418878aa329687dce3638a4
Reviewed-on: https://chromium-review.googlesource.com/776407
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519604}
[modify] https://crrev.com/d0d92c5d1765633b60e397df001cad5894935467/third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp
[modify] https://crrev.com/d0d92c5d1765633b60e397df001cad5894935467/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp
[modify] https://crrev.com/d0d92c5d1765633b60e397df001cad5894935467/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.h

Cc: hirosh...@chromium.org
How about RefCounted or GarbageCollectedFinalized?

My context is, for example:
https://codereview.chromium.org/2724673002/

Resource can be revalidated, and its users (e.g. ScriptResourceClients) might want to keep the old ResourceResponse before revalidation, for a while even after a new ResourceResponse is received during revalidation, because they don't want to be affected by revalidations.

In the CL above, ScriptResourceData copies ResourceResponse from Resource on NotifyFinished(), and ScriptResource's users uses the ResourceResponse in ScriptResourceData.

When ResourceResponse can be shared, then ScriptResource and ScriptResourceData can share the ResourceResponse, instead of copying.

The CL above is not committed due to other blocking issues, but I have similar motiviation in some other places e.g. for ImageResource.
Blockedon: 794651
I haven't understood all of your changes in the CL above and may miss something, but could you just move the ownership of ResourceResponse to ScriptResourceData? 

Since ScriptResource seems to own the ScriptResourceData, it's possible to access the ResourceResponse that ScriptResourceData owns only when it is necessary?
Cc: -tyoshino@chromium.org
Labels: Performance-Loading
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f7f9493df845d725f34cbf50e2437456d3be3bea

commit f7f9493df845d725f34cbf50e2437456d3be3bea
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Tue Mar 27 08:32:58 2018

OOR-CORS: Simplify DocumentThreadableLoader's preflight request creation code

Now DocumentThreadableLoader uses Blink API types to construct a
CORS-preflight request, but this isn't necessary.

Also, this change prefer using std::unique_ptr for ResourceRequest
so to respect the direction of crbug.com/787704.

Bug:  803766 , 787704
Change-Id: I897b17b7ea20e47316c05436c7d36438e03f2d7d
Reviewed-on: https://chromium-review.googlesource.com/964074
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546062}
[modify] https://crrev.com/f7f9493df845d725f34cbf50e2437456d3be3bea/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/f7f9493df845d725f34cbf50e2437456d3be3bea/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
[modify] https://crrev.com/f7f9493df845d725f34cbf50e2437456d3be3bea/third_party/WebKit/Source/core/loader/DocumentThreadableLoaderTest.cpp

Sign in to add a comment