Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814013004:
Avoid IPC message creation between URLLoaderClientImpl and ResourceDispatcher (Closed)
DescriptionAvoid IPC message creation between URLLoaderClientImpl and ResourceDispatcher
Currently we create an IPC message for each URLLoaderClient call for code
maintenanceability. This CL stops that for performance.
BUG=710969
Review-Url: https://codereview.chromium.org/2814013004
Cr-Commit-Position: refs/heads/master@{#464646}
Committed: https://chromium.googlesource.com/chromium/src/+/e45a888ffe01954af865afbb1b58020adafd5895
Patch Set 1 #
Total comments: 5
Patch Set 2 : fix #Patch Set 3 : fix #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yhirano@chromium.org changed reviewers: + kinuko@chromium.org, tzik@chromium.org
https://codereview.chromium.org/2814013004/diff/1/content/child/url_loader_cl... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2814013004/diff/1/content/child/url_loader_cl... content/child/url_loader_client_impl.cc:150: reinterpret_cast<const std::vector<char>&>(data); tzik@, is this OK?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Avoid IPC message creation between URLLoaderClientImpl and ResourceDispatcher Currently we create an IPC message for each URLLoaderClient call for code maintenanceability. This CL stops that for performance. BUG=603396 ========== to ========== Avoid IPC message creation between URLLoaderClientImpl and ResourceDispatcher Currently we create an IPC message for each URLLoaderClient call for code maintenanceability. This CL stops that for performance. BUG=710969 ==========
https://codereview.chromium.org/2814013004/diff/1/content/child/url_loader_cl... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2814013004/diff/1/content/child/url_loader_cl... content/child/url_loader_client_impl.cc:150: reinterpret_cast<const std::vector<char>&>(data); On 2017/04/12 13:29:27, yhirano (slow) wrote: > tzik@, is this OK? Oh... Can we avoid it by modifying the IPC? https://codereview.chromium.org/2814013004/diff/1/content/child/url_loader_cl... content/child/url_loader_client_impl.cc:202: NOTREACHED(); Can we DCHECK(is_deferred_ || !deferred_message_.empty()); around line 196?
The CQ bit was checked by kinuko@chromium.org
lgtm
The CQ bit was unchecked by kinuko@chromium.org
(Oops, wrongly hit LGTM and CQ...) Did this show up on any profile etc?
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2814013004/diff/1/content/child/url_loader_cl... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2814013004/diff/1/content/child/url_loader_cl... content/child/url_loader_client_impl.cc:150: reinterpret_cast<const std::vector<char>&>(data); On 2017/04/13 01:55:49, tzik wrote: > On 2017/04/12 13:29:27, yhirano (slow) wrote: > > tzik@, is this OK? > > Oh... Can we avoid it by modifying the IPC? We can, but it needs more work. OK, I removed this controversial change from this CL. https://codereview.chromium.org/2814013004/diff/1/content/child/url_loader_cl... content/child/url_loader_client_impl.cc:202: NOTREACHED(); On 2017/04/13 01:55:49, tzik wrote: > Can we DCHECK(is_deferred_ || !deferred_message_.empty()); around line 196? Done.
On 2017/04/13 03:21:46, kinuko wrote: > (Oops, wrongly hit LGTM and CQ...) > I'm confused, does that mean you want to add more comments? (context: I want to land this change before the branch point in order to include this change in the finch experiment in beta.) > Did this show up on any profile etc? No.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2017/04/13 14:19:05, yhirano (slow) wrote: > On 2017/04/13 03:21:46, kinuko wrote: > > (Oops, wrongly hit LGTM and CQ...) > > > I'm confused, does that mean you want to add more comments? > (context: I want to land this change before the branch point in order to include > this change in the finch experiment in beta.) l-g-t-m part was fine, cq bit was the one that was unexpected. And I wanted to wait how you address tzik's comment. (still lgtm) > > Did this show up on any profile etc? > No.
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492132557968570,
"parent_rev": "f44bb232631572abb14bc01f2c565af5e96f7d36", "commit_rev":
"e45a888ffe01954af865afbb1b58020adafd5895"}
Message was sent while issue was closed.
Description was changed from ========== Avoid IPC message creation between URLLoaderClientImpl and ResourceDispatcher Currently we create an IPC message for each URLLoaderClient call for code maintenanceability. This CL stops that for performance. BUG=710969 ========== to ========== Avoid IPC message creation between URLLoaderClientImpl and ResourceDispatcher Currently we create an IPC message for each URLLoaderClient call for code maintenanceability. This CL stops that for performance. BUG=710969 Review-Url: https://codereview.chromium.org/2814013004 Cr-Commit-Position: refs/heads/master@{#464646} Committed: https://chromium.googlesource.com/chromium/src/+/e45a888ffe01954af865afbb1b58... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e45a888ffe01954af865afbb1b58... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
