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

Issue 803125 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 598073



Sign in to add a comment

Make network service interface use lazy initialization and moveable structs

Project Member Reported by jam@chromium.org, Jan 17 2018

Issue description

We can do 2 things to eliminate copying and unnecessary serialization when URLLoader is used in process:
-add support_lazy_serialization to the GN target 
-make the big structs (ResourceRequest, ResourceResponse) moveable

This will improve performance for 2 key scenarios:
-when PlzNavigate uses URLLoader in-process when network service is disabled
-with direct renderer <> service worker communication

I'll do the first one now as it's quick before branch point to reduce the risk that NavigationMojoResponse causes a performance regression.
 

Comment 1 by jam@chromium.org, Jan 17 2018

Cc: falken@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 18 2018

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

commit c02aba509ea2b2d322758982f52739a2dcf09fa0
Author: John Abd-El-Malek <jam@chromium.org>
Date: Thu Jan 18 18:04:01 2018

Turn on lazy serialization for network service interfaces.

Two new code paths use in-process URLLoader & URLLoaderFactory: PlzNavigate with URLLoader and the new Service Worker networking glue rewrite. Avoid serializing the big structs in those cases.

We still want to make the structs be moveable to avoid copies in future cls.

Bug: 803125
Change-Id: I752e28f853967c28786b9d3f50e57a93ed662f89
Reviewed-on: https://chromium-review.googlesource.com/874294
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530199}
[modify] https://crrev.com/c02aba509ea2b2d322758982f52739a2dcf09fa0/services/network/public/interfaces/BUILD.gn

Comment 3 by yzshen@chromium.org, Jan 18 2018

Cc: -falken@chromium.org xunji...@chromium.org
One thing that should be noted:
In order to reduce data copies, Helen's pending CL of UDP support uses mojo.common.common.mojom.ReadOnlyBuffer, which is mapped to base::span<const uint8_t> and directly refers to bytes within the received message. It doesn't work with lazy serialization.

I think a quick solution is to put the UDP socket interface in a separate target which doesn't turn on lazy initialization.

Eventually we should support lazy-serialization for ReadOnlyBuffer, which requires introducing some new typemap annotation for different storage/parameter types, I think.

Comment 4 by yzshen@chromium.org, Jan 18 2018

Cc: falken@chromium.org
Sorry, mistakenly removed falken@ from CC list. I have added back.

Comment 5 by mek@chromium.org, Jan 26 2018

Another thing that should be noted is that with lazy serialization WTF::String and KURL can no longer be safely passed to mojo methods either, without first creating isolated copies (this is currently causing flaky crashes such as https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel_ng/636935/layout-test-results/results.html)
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 31 2018

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

commit c9c268a9d7f28a758064d1ac8f91190b82617933
Author: Ken Rockot <rockot@chromium.org>
Date: Wed Jan 31 21:04:40 2018

Add [force_serialize] mojom typemap attribute

This allows a type mapping to specify that its mapped type must be
serialized when sent, i.e. it can never be copied or moved into
intermediate storage for lazy serialization. Messages which depend
on such types thus automatically have lazy serialization disabled and do
not generate any lazy serialization code.

TBR=dcheng@chromium.org

Bug: 803125
Change-Id: I4a39a4ec24a665fa4f6dae6459f57673d1485f67
Reviewed-on: https://chromium-review.googlesource.com/894603
Commit-Queue: Ken Rockot <rockot@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Yuzhu Shen <yzshen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533407}
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/mojo/common/read_only_buffer.typemap
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/mojo/public/cpp/bindings/README.md
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/mojo/public/cpp/bindings/tests/lazy_serialization_unittest.cc
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/mojo/public/cpp/bindings/tests/struct_with_traits.typemap
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/mojo/public/cpp/bindings/tests/struct_with_traits_impl.cc
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/mojo/public/cpp/bindings/tests/struct_with_traits_impl.h
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/mojo/public/cpp/bindings/tests/struct_with_traits_impl_traits.h
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/mojo/public/interfaces/bindings/tests/struct_with_traits.mojom
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/mojo/public/tools/bindings/generate_type_mappings.py
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/mojo/public/tools/bindings/generators/mojom_cpp_generator.py
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/third_party/WebKit/Source/platform/mojo/KURL.typemap
[modify] https://crrev.com/c9c268a9d7f28a758064d1ac8f91190b82617933/third_party/WebKit/Source/platform/mojo/String.typemap

Cc: -rdsmith@chromium.org
Cc: -roc...@chromium.org rockot@google.com
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 7

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

commit e9bf5c8011e6c35cb361e623726cbe969a7a2d92
Author: John Abd-El-Malek <jam@chromium.org>
Date: Mon Jan 07 22:39:14 2019

Disable lazy serialization in network interfaces.

Per discussion in https://chromium-review.googlesource.com/c/chromium/src/+/1374957, we don't see a
performance gain from enabling them. Since this has a binary size cost, disable it.

Bug: 803125

Change-Id: I03090214fdb7ecba9e987065e62ca2a1d6dfd430
Reviewed-on: https://chromium-review.googlesource.com/c/1391913
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620507}
[modify] https://crrev.com/e9bf5c8011e6c35cb361e623726cbe969a7a2d92/services/network/public/mojom/BUILD.gn

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 9

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

commit 007850b95fe68fc29f2d615500d9a4e31d530da3
Author: François Doray <fdoray@chromium.org>
Date: Wed Jan 09 15:20:59 2019

Revert "Disable lazy serialization in network interfaces."

This reverts commit e9bf5c8011e6c35cb361e623726cbe969a7a2d92.

Reason for revert: Tentative fix for  https://crbug.com/919929 .

Original change's description:
> Disable lazy serialization in network interfaces.
>
> Per discussion in https://chromium-review.googlesource.com/c/chromium/src/+/1374957, we don't see a
> performance gain from enabling them. Since this has a binary size cost, disable it.
>
> Bug: 803125
>
> Change-Id: I03090214fdb7ecba9e987065e62ca2a1d6dfd430
> Reviewed-on: https://chromium-review.googlesource.com/c/1391913
> Reviewed-by: Ken Rockot <rockot@google.com>
> Commit-Queue: John Abd-El-Malek <jam@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#620507}

TBR=jam@chromium.org,rockot@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 803125,  919929 
Change-Id: Ib6224258071d6716555fab185f8034f1081a5af3
Reviewed-on: https://chromium-review.googlesource.com/c/1403055
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621145}
[modify] https://crrev.com/007850b95fe68fc29f2d615500d9a4e31d530da3/services/network/public/mojom/BUILD.gn

Project Member

Comment 11 by bugdroid1@chromium.org, Jan 9

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

commit d40f488e0b0e47b79d817dafafdf07e450e17717
Author: John Abd-El-Malek <jam@chromium.org>
Date: Wed Jan 09 17:58:41 2019

Reland "Disable lazy serialization in network interfaces."

This is a reland of e9bf5c8011e6c35cb361e623726cbe969a7a2d92

The failing test was disabled in r621196.

Original change's description:
> Disable lazy serialization in network interfaces.
>
> Per discussion in https://chromium-review.googlesource.com/c/chromium/src/+/1374957, we don't see a
> performance gain from enabling them. Since this has a binary size cost, disable it.
>
> Bug: 803125
>
> Change-Id: I03090214fdb7ecba9e987065e62ca2a1d6dfd430
> Reviewed-on: https://chromium-review.googlesource.com/c/1391913
> Reviewed-by: Ken Rockot <rockot@google.com>
> Commit-Queue: John Abd-El-Malek <jam@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#620507}

Bug: 803125
Change-Id: I9c267ff429275f11406f2ab61eb52abbce62f15f
TBR: rockot@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/1403185
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621215}
[modify] https://crrev.com/d40f488e0b0e47b79d817dafafdf07e450e17717/services/network/public/mojom/BUILD.gn

Status: Untriaged (was: Available)
Available, but no owner or component? Please find a component, as no one will ever find this without one.

Sign in to add a comment