Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1401813002:
Cache User Agent string in BlinkPlatformImpl (Closed)
|
Created:
5 years, 2 months ago by tzik Modified:
5 years, 1 month ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, Inactive, chromium-reviews, devtools-reviews_chromium.org, gavinp+loader_chromium.org, kinuko+watch, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, tyoshino+watch_chromium.org, vivekg_samsung, vivekg, yurys+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCache User Agent string in BlinkPlatformImpl
Issuing resource requests take around 10% of time-to-first-paint on typical discarded tab restoration.
And UA string calculation takes 30% of time of above.
This CL add a cache of UA string in Platform to cut 3% of the time.
BUG=540988
Committed: https://crrev.com/d15db75602e43a8f6bfd480a9442745544cac18f
Cr-Commit-Position: refs/heads/master@{#356813}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : moved to FrameLoaderClientImpl #
Total comments: 4
Patch Set 5 : Moved to FrameLoader #Patch Set 6 : +comment #Patch Set 7 : Moved to BlinkPlatformImpl #Patch Set 8 : +comment. +DCHECK #Messages
Total messages: 65 (25 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401813002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401813002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
tzik@chromium.org changed reviewers: + kouhei@chromium.org
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401813002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401813002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401813002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401813002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401813002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401813002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tzik@chromium.org changed reviewers: + kinuko@chromium.org
PTAL
Great work! Can we do this at blink level? I was wondering if creating a new WebString each time is still time consuming.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401813002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401813002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/10/14 11:44:29, kouhei wrote: > Great work! > Can we do this at blink level? I was wondering if creating a new WebString each > time is still time consuming. OK, I moved it to FrameLoaderClientImpl.
Can we add a DCHECK to SetContentClient()? Description: issueing -> issuing
lgtm, assuming the memory usage for caching this is negligible https://codereview.chromium.org/1401813002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/1401813002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:690: m_userAgent = Platform::current()->userAgent(); Maybe add a comment in Platform.h to note that the return value of userAgent() is not expected to be changed and can be cached. https://codereview.chromium.org/1401813002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FrameLoaderClientImpl.h (right): https://codereview.chromium.org/1401813002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.h:189: WebString m_userAgent; nit: let's just use String in non-Web / boundary classes
lgtm % kinuko-san's comments
On 2015/10/16 11:58:38, kouhei wrote: > lgtm % kinuko-san's comments (drive-by) Adding new logic to FrameLoadClientImpl is undesirable, it should ideally just be plumbing. Could we put it in FrameLoader instead? :)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401813002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401813002/100001
On 2015/10/16 18:51:15, Nate Chapin wrote: > On 2015/10/16 11:58:38, kouhei wrote: > > lgtm % kinuko-san's comments > > (drive-by) > > Adding new logic to FrameLoadClientImpl is undesirable, it should ideally just > be plumbing. Could we put it in FrameLoader instead? :) Moved it to FrameLoader and split FrameLoaderClient::userAgent into two.
tzik@chromium.org changed reviewers: + japhet@chromium.org
Updated. Kinuko-san, Kouhei-san. Could you also take another look to this? https://codereview.chromium.org/1401813002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/1401813002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:690: m_userAgent = Platform::current()->userAgent(); On 2015/10/16 11:12:21, kinuko wrote: > Maybe add a comment in Platform.h to note that the return value of userAgent() > is not expected to be changed and can be cached. Done. https://codereview.chromium.org/1401813002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FrameLoaderClientImpl.h (right): https://codereview.chromium.org/1401813002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.h:189: WebString m_userAgent; On 2015/10/16 11:12:21, kinuko wrote: > nit: let's just use String in non-Web / boundary classes Acknowledged. I moved to this to FrameLoader and changed its type to String.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
looks like It breaks tests...
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
This doesn't seem like the right layer for the cache, why isn't the cache out in the client where userAgent() is implemented?
On 2015/10/20 02:46:52, esprehn wrote: > This doesn't seem like the right layer for the cache, why isn't the cache out in > the client where userAgent() is implemented? The reason we don't want to do this in content/ is to avoid WTFString creation overhead. This is in the hot path for all resource requests from Blink, and especially affects cached resource load performance.
On 2015/10/20 at 03:35:25, kouhei wrote: > On 2015/10/20 02:46:52, esprehn wrote: > > This doesn't seem like the right layer for the cache, why isn't the cache out in > > the client where userAgent() is implemented? > > The reason we don't want to do this in content/ is to avoid WTFString creation overhead. > This is in the hot path for all resource requests from Blink, and especially affects cached resource load performance. That's fine, Platform::userAgent() returns a WebString, you can put a static local WebString in BlinkPlatformImpl that caches the WebString object which means we won't be copying any strings. You just need in BlinkPlatformImpl: CR_DEFINE_STATIC_LOCAL(WebString, kUserAgent, (...)); return kUserAgent;
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401813002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401813002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2015/10/20 03:55:24, esprehn wrote: > On 2015/10/20 at 03:35:25, kouhei wrote: > > On 2015/10/20 02:46:52, esprehn wrote: > > > This doesn't seem like the right layer for the cache, why isn't the cache > out in > > > the client where userAgent() is implemented? > > > > The reason we don't want to do this in content/ is to avoid WTFString creation > overhead. > > This is in the hot path for all resource requests from Blink, and especially > affects cached resource load performance. > > That's fine, Platform::userAgent() returns a WebString, you can put a static > local WebString in BlinkPlatformImpl that caches the WebString object which > means we won't be copying any strings. > > > > You just need in BlinkPlatformImpl: > > CR_DEFINE_STATIC_LOCAL(WebString, kUserAgent, (...)); > return kUserAgent; Ok, I moved it back to BlinkPlatformImpl with as a static local. PTAL.
still lgtm
lgtm.
Maybe you've overlooked my comments as they're not inlined ones. On 2015/10/16 06:29:45, tyoshino wrote: > Can we add a DCHECK to SetContentClient()? I was just wondering if we can do this. Optional. > > Description: issueing -> issuing Please fix. --- I think the comment you've added in ps6 in response to kinuko@'s comment is still useful. Can we keep it?
lgtm
On 2015/10/29 at 04:14:32, esprehn wrote: > lgtm Btw you need to update your patch description and title.
Description was changed from ========== Cache User Agent string in FrameLoaderClientImpl Issueing resource requests take around 10% of time-to-first-paint on typical discarded tab restoration. And UA string calculation takes 30% of time of above. This CL add a cache of UA string in Platform to cut 3% of the time. BUG=540988 ========== to ========== Cache User Agent string in BlinkPlatformImpl Issueing resource requests take around 10% of time-to-first-paint on typical discarded tab restoration. And UA string calculation takes 30% of time of above. This CL add a cache of UA string in Platform to cut 3% of the time. BUG=540988 ==========
Description was changed from ========== Cache User Agent string in BlinkPlatformImpl Issueing resource requests take around 10% of time-to-first-paint on typical discarded tab restoration. And UA string calculation takes 30% of time of above. This CL add a cache of UA string in Platform to cut 3% of the time. BUG=540988 ========== to ========== Cache User Agent string in BlinkPlatformImpl Issuing resource requests take around 10% of time-to-first-paint on typical discarded tab restoration. And UA string calculation takes 30% of time of above. This CL add a cache of UA string in Platform to cut 3% of the time. BUG=540988 ==========
On 2015/10/29 04:13:22, tyoshino wrote: > Maybe you've overlooked my comments as they're not inlined ones. > > On 2015/10/16 06:29:45, tyoshino wrote: > > Can we add a DCHECK to SetContentClient()? > > I was just wondering if we can do this. Optional. > > > > > Description: issueing -> issuing > > Please fix. > > --- > > I think the comment you've added in ps6 in response to kinuko@'s comment is > still useful. Can we keep it? Ah, yes, I overlooked yours. I added a comment and a DCHECK for UA string change.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401813002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401813002/140001
tyoshino@chromium.org changed reviewers: + tyoshino@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, esprehn@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/1401813002/#ps140001 (title: "+comment. +DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401813002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401813002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/d15db75602e43a8f6bfd480a9442745544cac18f Cr-Commit-Position: refs/heads/master@{#356813} |
