New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 766653 link

Starred by 4 users
Status: WontFix
Owner:
japhet@chromium.org
Closed: Dec 21
Cc:
kinuko@chromium.org
kouhei@chromium.org
haraken@chromium.org
█ vdje...@fb.com
rmcilroy@chromium.org
hirosh...@chromium.org
toyoshim@chromium.org
█ n...@fb.com
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

MemoryCache: Experiment keeping strong refs to document resources during navigation

Project Member Reported by kinuko@chromium.org, Sep 19 2017 
Current MemoryCache's behavior is a bit unpredictable in the way that developers can't really control, especially during navigation. Given that experimenting a few different behaviors don't look too difficult let's experiment what if we explicitly keep strong refs to Document's resources during navigation.

Candidate timings to clear references we've discussed:
- onload
- on parser finished
- (forever- won't try this)

More details about what we've discussed can be found on this doc:
https://docs.google.com/document/d/1aTPmpe-cCDW7VfExN-fjMTe7eU9EwBcOW4VjVx2zOSE/edit?ts=59b1cd79
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 20 2017 

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

commit 9638e7100016d78505b4e9e872f66c123bd8dbe5
Author: Nate Chapin <japhet@chromium.org>
Date: Wed Sep 20 22:11:01 2017

Add a flag to keep the previous document's resources alive until the
current document has reached a given point of load completion.

Bug:  766653 
Change-Id: I1d9e9964e8653d5ea2449a164b77a3cd32794a1d
Reviewed-on: https://chromium-review.googlesource.com/667964
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Kinuko Yasuda (slow) <kinuko@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503266}
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/chrome/browser/about_flags.cc
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/content/common/content_switches_internal.cc
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/content/common/content_switches_internal.h
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/content/public/common/common_param_traits_macros.h
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/content/public/common/content_switches.cc
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/content/public/common/content_switches.h
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/content/public/common/web_preferences.cc
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/content/public/common/web_preferences.h
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/content/renderer/render_view_impl.cc
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/Source/core/exported/WebSettingsImpl.cpp
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/Source/core/exported/WebSettingsImpl.h
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/Source/core/frame/Settings.json5
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/Source/core/loader/FrameFetchContext.h
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/Source/core/loader/FrameLoaderTypes.h
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/third_party/WebKit/public/web/WebSettings.h
[modify] https://crrev.com/9638e7100016d78505b4e9e872f66c123bd8dbe5/tools/metrics/histograms/enums.xml

Comment 2 by kouhei@chromium.org, Sep 20 2017

Cc: n...@fb.com vdje...@fb.com
+cc: Facebook folks who were interested in the experiment.

Comment 3 by n...@fb.com, Sep 25 2017

Labels: DevRel-Facebook

Comment 4 by kinuko@chromium.org, Sep 28 2017 

Per our off-line discussion:
Let's add one more variation to the experiment for: not to retain resources into the new ResourceFetcher, or drop them if/when we can know it’s going to be cross-site navigation. The large concern here is that retaining resources during navigation could increase peak memory usage during navigation, and another observation is that we will unlikely need the resources (or the process will likely be just killed) if the navigation is cross-origin.

Comment 5 by ricea@chromium.org, Sep 28 2017

Components: Blink>Loader
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 4 2017 

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

commit d48d5c04a44299357a82656c0015f95dea4a4a92
Author: Nate Chapin <japhet@chromium.org>
Date: Wed Oct 04 18:15:51 2017

Only save resources from previous ResourceFetcher for same-origin non-import navs

When the savePreviousDocumentResources experiment is enabled, don't bother saving
resources for cross-origin navigations, since they are more likely to end up in
a different process, and will be very unlikely to share resources with the
current Document anyway.

Bug:  766653 
Change-Id: I7facbf4239e482f11df8602ce72a5f1908eafd9a
Reviewed-on: https://chromium-review.googlesource.com/692679
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506445}
[modify] https://crrev.com/d48d5c04a44299357a82656c0015f95dea4a4a92/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp

Comment 7 by rmcilroy@chromium.org, Nov 9 2017

Cc: rmcilroy@chromium.org
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 1 2017 

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

commit 82a20b628c47a5c465af8f035935ecdbf3ce1689
Author: Nate Chapin <japhet@chromium.org>
Date: Fri Dec 01 17:09:24 2017

Add a Feature and field trial for SavePreviousDocumentResources experiment

Bug:  766653 
Change-Id: Iff945c3b637fc566380b6ab8e2ab8e1c409088ba
Reviewed-on: https://chromium-review.googlesource.com/756196
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520966}
[modify] https://crrev.com/82a20b628c47a5c465af8f035935ecdbf3ce1689/content/common/content_switches_internal.cc
[modify] https://crrev.com/82a20b628c47a5c465af8f035935ecdbf3ce1689/testing/variations/fieldtrial_testing_config.json

Comment 9 by kinuko@chromium.org, Dec 19 2017

Labels: -Pri-3 Pri-2
Slightly bumping up a priority given that this could be blocking a memory leak fix to land.

Comment 10 by kinuko@chromium.org, Dec 20 2017

Cc: toyoshim@chromium.org
Small update: last-minutes server-side change for experiment is landed, though the initial metrics are a bit rough.

Comment 11 by ojan@chromium.org, May 8 2018

Cc: -ojan@chromium.org
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 21 

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

commit d3aa5e366ada1e951c196adc0918729d24f3e36b
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Fri Dec 21 02:41:35 2018

Remove experimental "SavePreviousDocumentResources" code

We haven't seen a significant improvment, so this CL deletes the code.

Bug:  766653 
Change-Id: Ibecb4582ba5c24bd20ade349c8c9a5de11fc837c
Reviewed-on: https://chromium-review.googlesource.com/c/1385698
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618438}
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/chrome/browser/about_flags.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/content/common/content_switches_internal.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/content/common/content_switches_internal.h
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/content/public/common/common_param_traits_macros.h
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/content/public/common/content_switches.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/content/public/common/content_switches.h
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/content/public/common/web_preferences.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/content/public/common/web_preferences.h
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/content/renderer/render_view_impl.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/testing/variations/fieldtrial_testing_config.json
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/third_party/blink/public/web/web_settings.h
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/third_party/blink/renderer/core/exported/web_settings_impl.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/third_party/blink/renderer/core/exported/web_settings_impl.h
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/third_party/blink/renderer/core/frame/settings.json5
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/third_party/blink/renderer/core/loader/document_loader.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/third_party/blink/renderer/core/loader/frame_fetch_context.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
[modify] https://crrev.com/d3aa5e366ada1e951c196adc0918729d24f3e36b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h

Comment 13 by yhirano@chromium.org, Dec 21

Status: WontFix (was: Started)
I removed the code for the experiment.
►

Sign in to add a comment