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

Issue 707953 link

Starred by 1 user
Status: Fixed
Owner: ----
Closed: May 2017
Cc:
zhongyi@chromium.org
█ xunji...@chromium.org
rch@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Refactor QuicStreamFactory::|active_jobs_| to not be a map of map

Project Member Reported by xunji...@chromium.org, Apr 3 2017 
QuicStreamFactory's |active_jobs_| is a std::map of std::map (std::map<QuicServerId, JobMap>). The JobMap seems to have only 2 or fewer elements (one main Job and one auxiliary Job to skip loading QuicServerInfo). Using a std::map to hold 2 elements seems to be an overkill, considering std::map is also very memory inefficient (https://docs.google.com/document/d/1YL1FORFMWo0FK0lMg7WsImnjNQ3ZpY0nK0NHGjkeHT4/edit?usp=sharing)


Can we refactor the container? Maybe a map of pairs of main job and aux job? That way, we can also add checks to make sure we are expecting <= 2 Jobs for a QuicServerId.

Comment 1 by zhongyi@chromium.org, Apr 3 2017

Cc: rch@chromium.org
+1. That's a good idea! I did notice this when I was debugging the memory leak issue. auxiliary Job is used in an experiment where we try to race loading server info from disk or simply doing non-zero RTT if I remembered the code correctly. 

I felt changing the map to a pair of jobs should be okay, unless we expect the number of jobs could grow > 2.

rch@: do you have any concern to change this?

Comment 2 by rch@chromium.org, Apr 3 2017 

SGTM. We have a flat_map (or small_map?) container for just this sort of purpose, iirc.

Comment 3 by xunji...@chromium.org, Apr 4 2017 

base::flat_map and base::flat_set are recently added. Both should be fine. Though I would prefer having a container that can enforce stronger guarantees on the number of QuicStreamFactory::Jobs per QuicServerId.  This will also help with the readability of the code -- it isn't obvious why |active_jobs_| needs to be a map of map when I'd assumed that there was a one-to-one mapping from QuicServerId to Job.

Is the experiment still needed?
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 19 2017 

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

commit 6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda
Author: rch <rch@chromium.org>
Date: Wed Apr 19 02:46:23 2017

Remove the code to store and load QUIC server configs in the disk cache.

This includes the load_server_info_time_to_srtt, enable_connection_racing and disable_disk_cache finch options.

BUG= 707953 

Review-Url: https://codereview.chromium.org/2820573004
Cr-Commit-Position: refs/heads/master@{#465474}

[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/components/network_session_configurator/network_session_configurator.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/components/network_session_configurator/network_session_configurator_unittest.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/BUILD.gn
[delete] https://crrev.com/9fca3869dc3c539cecdea9e7934499c0ac493b9e/net/http/disk_cache_based_quic_server_info.cc
[delete] https://crrev.com/9fca3869dc3c539cecdea9e7934499c0ac493b9e/net/http/disk_cache_based_quic_server_info.h
[delete] https://crrev.com/9fca3869dc3c539cecdea9e7934499c0ac493b9e/net/http/disk_cache_based_quic_server_info_unittest.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/http/http_cache.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/http/http_network_session.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/http/http_network_session.h
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/properties_based_quic_server_info.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/properties_based_quic_server_info.h
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/properties_based_quic_server_info_test.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/quic_server_info.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/quic_server_info.h
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/quic_stream_factory.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/quic_stream_factory.h
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/quic_stream_factory_peer.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/quic_stream_factory_peer.h
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/quic/chromium/quic_stream_factory_test.cc
[modify] https://crrev.com/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda/net/url_request/url_request_unittest.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 19 2017 

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

commit 5644040ec767fafe48393e5186901da9452464d7
Author: aboxhall <aboxhall@chromium.org>
Date: Wed Apr 19 05:46:23 2017

Revert of Remove the code to store and load QUIC server configs in the disk cache. (patchset #13 id:240001 of https://codereview.chromium.org/2820573004/ )

Reason for revert:
Lots of MSan failures:
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20ChromiumOS%20MSan%20Tests/builds/89/steps/net_unittests

Original issue's description:
> Remove the code to store and load QUIC server configs in the disk cache.
>
> This includes the load_server_info_time_to_srtt, enable_connection_racing and disable_disk_cache finch options.
>
> BUG= 707953 
>
> Review-Url: https://codereview.chromium.org/2820573004
> Cr-Commit-Position: refs/heads/master@{#465474}
> Committed: https://chromium.googlesource.com/chromium/src/+/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda

TBR=ianswett@google.com,jri@chromium.org,rch@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 707953 

Review-Url: https://codereview.chromium.org/2821053004
Cr-Commit-Position: refs/heads/master@{#465502}

[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/components/network_session_configurator/network_session_configurator.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/components/network_session_configurator/network_session_configurator_unittest.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/BUILD.gn
[add] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/http/disk_cache_based_quic_server_info.cc
[add] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/http/disk_cache_based_quic_server_info.h
[add] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/http/disk_cache_based_quic_server_info_unittest.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/http/http_cache.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/http/http_network_session.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/http/http_network_session.h
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/properties_based_quic_server_info.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/properties_based_quic_server_info.h
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/properties_based_quic_server_info_test.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/quic_server_info.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/quic_server_info.h
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/quic_stream_factory.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/quic_stream_factory.h
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/quic_stream_factory_peer.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/quic_stream_factory_peer.h
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/quic/chromium/quic_stream_factory_test.cc
[modify] https://crrev.com/5644040ec767fafe48393e5186901da9452464d7/net/url_request/url_request_unittest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 19 2017 

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

commit 431dd44543668f59e341aaf350f1370690ee9b35
Author: rch <rch@chromium.org>
Date: Wed Apr 19 15:22:35 2017

Remove the code to store and load QUIC server configs in the disk cache.

This includes the load_server_info_time_to_srtt, enable_connection_racing and disable_disk_cache finch options.

BUG= 707953 

Review-Url: https://codereview.chromium.org/2820573004
Cr-Original-Commit-Position: refs/heads/master@{#465474}
Committed: https://chromium.googlesource.com/chromium/src/+/6f3dd74bddb23a2e5e74ba6976f02f94d76d2dda
Review-Url: https://codereview.chromium.org/2820573004
Cr-Commit-Position: refs/heads/master@{#465613}

[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/components/network_session_configurator/network_session_configurator.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/components/network_session_configurator/network_session_configurator_unittest.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/BUILD.gn
[delete] https://crrev.com/c8eaca63b6cd1d78854e493bae4eb18ccaecfcc2/net/http/disk_cache_based_quic_server_info.cc
[delete] https://crrev.com/c8eaca63b6cd1d78854e493bae4eb18ccaecfcc2/net/http/disk_cache_based_quic_server_info.h
[delete] https://crrev.com/c8eaca63b6cd1d78854e493bae4eb18ccaecfcc2/net/http/disk_cache_based_quic_server_info_unittest.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/http/http_cache.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/http/http_network_session.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/http/http_network_session.h
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/properties_based_quic_server_info.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/properties_based_quic_server_info.h
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/properties_based_quic_server_info_test.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/quic_server_info.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/quic_server_info.h
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/quic_stream_factory.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/quic_stream_factory.h
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/quic_stream_factory_peer.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/quic_stream_factory_peer.h
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/quic/chromium/quic_stream_factory_test.cc
[modify] https://crrev.com/431dd44543668f59e341aaf350f1370690ee9b35/net/url_request/url_request_unittest.cc

Comment 7 by zhongyi@chromium.org, Apr 25 2017

Cc: zhongyi@chromium.org xunji...@chromium.org
 Issue 715229  has been merged into this issue.
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 26 2017 

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

commit bc11943ed50cef962cc059b6e24493f84c4de5fa
Author: zhongyi <zhongyi@chromium.org>
Date: Wed Apr 26 18:01:28 2017

Change QuicStreamFactory::active_jobs_ to a Map from QuicServerId to Job
as auxiliary Job is no longer supported. There should be at most one Job
serving the same QuicServerId.

BUG= 707953 

Review-Url: https://codereview.chromium.org/2842883002
Cr-Commit-Position: refs/heads/master@{#467380}

[modify] https://crrev.com/bc11943ed50cef962cc059b6e24493f84c4de5fa/net/log/net_log_event_type_list.h
[modify] https://crrev.com/bc11943ed50cef962cc059b6e24493f84c4de5fa/net/quic/chromium/quic_stream_factory.cc
[modify] https://crrev.com/bc11943ed50cef962cc059b6e24493f84c4de5fa/net/quic/chromium/quic_stream_factory.h
[modify] https://crrev.com/bc11943ed50cef962cc059b6e24493f84c4de5fa/net/quic/chromium/quic_stream_factory_peer.cc

Comment 9 by zhongyi@chromium.org, May 12 2017

Status: Fixed (was: Untriaged)
With all the changes above, QuicStreamFactory::active_jobs_ is a 1-1 map from QuicServerId to Job. Mark this bug as fixed.
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 6 2017 

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

commit 6959e579e9a5baf79ca77fd9024aa4efa59098f9
Author: Helen Li <xunjieli@chromium.org>
Date: Wed Sep 06 17:20:44 2017

Remove unused QuicStreamFactory::Job::Cancel()

The method is added in crrev.com/881133004 for racing two
QuicStreamFactory::Jobs. This logic has since been removed in
crrev.com/2820573004.

This CL removes the unused method.

Bug:  762562 ,  707953 
Change-Id: I2393713d816364e1002e75829cb6ade3873ad68d
Reviewed-on: https://chromium-review.googlesource.com/652959
Commit-Queue: Helen Li <xunjieli@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499996}
[modify] https://crrev.com/6959e579e9a5baf79ca77fd9024aa4efa59098f9/net/quic/chromium/quic_stream_factory.cc
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 6 2017 

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

commit b1427ee089099027ffd836d8f651fcb878a8ce4d
Author: Helen Li <xunjieli@chromium.org>
Date: Wed Sep 06 18:30:00 2017

depecate an obsolete QUIC histogram

Net.QuicServerInfo.DiskCacheWaitForDataReadyTime code is removed in
crrev.com/431dd44543668f59e341aaf350f1370690ee9b35.

This CL mark the histogram as deprecated.

Bug:  707953 
Change-Id: I43ed5f2556dfaa8924a4d6049c15c50669933149
Reviewed-on: https://chromium-review.googlesource.com/652963
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500014}
[modify] https://crrev.com/b1427ee089099027ffd836d8f651fcb878a8ce4d/tools/metrics/histograms/histograms.xml
►

Sign in to add a comment