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

Issue 842677 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug-Regression



Sign in to add a comment

3.6% regression in cronet_perf_tests at 558091:558095

Project Member Reported by xunji...@chromium.org, May 14 2018

Issue description

https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICQ_pqfpQsM

Looks like r558094 caused a regression in Cronet's upload performance on Android.

+rch, zhongyi@: Can you triage?
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, May 14 2018

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=842677

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=101d244c85d0d20c12aaa291c68e3e56f15d44fb4d2565d50e14fb4b11bbead2


Bot(s) for this bug's original alert(s):

android_cronet_m64_perf
Components: Internals>Network>Library
Labels: -Pri-2 Pri-1
r558094 is a recent merge CL which includes several changes. What in particular does the this test measure? Can you say more? I only saw the unit for the vertical axis but didn't understand what's been measured. 

How often does this bot run? Is that running for every commit? Can we run the bot manually w/o r558094 to confirm the regression? 
Note that this is not a Chrome perf benchmark. This is specifically for Cronet. You can run the test suite locally to confirm the regression. Instructions are at: https://cs.chromium.org/chromium/src/components/cronet/android/test/javaperftests/run.py

The vertical axis is average time taken for Cronet upload.

Interestingly, we also observed ~30% improvement over download speed: https://chromeperf.appspot.com/report?sid=2c7a06f055bc40189a8c12a40ad79f6023fe1af80891510adaf8c44179c5ef9c&start_rev=557765&end_rev=558484

Thanks for the clarification. 

r558094 is a merge landing CL, there are 6 CLs included:
- 3 of them are flag deprecation CLs, which doesn't have any feature change
  FLAGS_quic_reloadable_flag_quic_reset_stream_is_not_zombie,
  FLAGS_quic_reloadable_flag_quic_fast_path_on_stream_data_acked,
  FLAGS_quic_reloadable_flag_quic_free_mem_slice_out_of_order
- 2 flag protected CL, where flags are still off, shouldn't introduce feature change
  FLAGS_quic_reloadable_flag_quic_enable_h2_deframer,
  FLAGS_quic_reloadable_flag_quic_bbr_slower_startup2
A few flags has been flipped, which might be related to the regression(https://chromium-review.googlesource.com/c/chromium/src/+/1055888):
// If true, simplify pacing sender logic.
QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_simplify_pacing_sender, true)
// If true, respect IETF QUIC header format.
QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_respect_ietf_header, true)
// If true, detect losses from last largest lost packet number.
QUIC_FLAG(bool,
          FLAGS_quic_reloadable_flag_quic_incremental_loss_detection,
          true);
// If true, enable fast path in QuicStreamSendBuffer::FreeMemSlices.
QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_fast_free_mem_slice, true)
// Default enables QUIC ack decimation and adds a connection option to disable
// it.
QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_enable_ack_decimation, true)
// If true, declare in flight packets lost via early retransmit, even though it
// has no retransmittable frames.
QUIC_FLAG(
    bool,
 FLAGS_quic_reloadable_flag_quic_early_retransmit_detects_in_flight_packet_lost,
    true)
Cc: rch@chromium.org
Owner: fayang@chromium.org
+fayang@: Fan, could you take a close look on those flags and spot which might be the related flag? Thanks!

Project Member

Comment 7 by bugdroid1@chromium.org, May 15 2018

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

commit bd469bd763218e751bdbc63f90f435af1a39f663
Author: Zhongyi Shi <zhongyi@chromium.org>
Date: Tue May 15 23:55:17 2018

Flip back recent quic flags to verify cronet perf regression.

Suspected flags are flipped back to see if the cronet perf regression is fixed:
FLAGS_quic_reloadable_flag_quic_simplify_pacing_sender,
FLAGS_quic_reloadable_flag_quic_incremental_loss_detection,
FLAGS_quic_reloadable_flag_quic_enable_ack_decimation,
FLAGS_quic_reloadable_flag_quic_early_retransmit_detects_in_flight_packet_lost.

Bug: 842677
Change-Id: I86bbd84988de2f2eb5933d58d9c43c7a12a88b02
Reviewed-on: https://chromium-review.googlesource.com/1060426
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Zhongyi Shi <zhongyi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558890}
[modify] https://crrev.com/bd469bd763218e751bdbc63f90f435af1a39f663/net/third_party/quic/core/quic_flags_list.h

The commit in #7 significantly (+31.2%) reduced the QUIC download performance.

See: https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICQwYqn_QgM



Project Member

Comment 9 by bugdroid1@chromium.org, May 16 2018

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

commit c19c5d95ec5fadaec00d76b0e4d36c107e3e4464
Author: Fan Yang <fayang@chromium.org>
Date: Wed May 16 15:28:36 2018

Turn on some quic flags to see whether they are related with cronet perf regression: FLAGS_quic_reloadable_flag_quic_simplify_pacing_sender, FLAGS_quic_reloadable_flag_quic_incremental_loss_detection, FLAGS_quic_reloadable_flag_quic_early_retransmit_detects_in_flight_packet_lost.

R=rch@chromium.org

Bug: 842677
Change-Id: Ib31af2a2da05400bddbe1f852f5ca967dcfad8bf
Reviewed-on: https://chromium-review.googlesource.com/1061595
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Fan Yang <fayang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559109}
[modify] https://crrev.com/c19c5d95ec5fadaec00d76b0e4d36c107e3e4464/net/third_party/quic/core/quic_flags_list.h

Re #8: this is expected as the reverted CL caused a 30% downgrade in upload performance and 30% improvement in download perf. We are reverting the CL to get to the bottom to the upload issue.

Fan will keep working on this issue to spot the flag which caused the regression.
https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICQ_pqfpQsM

Looks like the spotted flag is: FLAGS_quic_reloadable_flag_quic_enable_ack_decimation.

Comment 12 by fayang@google.com, May 16 2018

Agreed. Thanks, Cherie. Assign this to Ian to see whether this is expected or a fix is needed (given the RTT of the test should be tiny).
Cc: fayang@chromium.org
Labels: -Pri-1 Pri-3
Owner: ianswett@chromium.org
Fan: before you decide to flip on the flag, you may want to explicitly exclude this flag in flag update script so that future flags update won't accidentally flip this flag. Thanks!

Comment 15 by rch@chromium.org, May 17 2018

zhongyi: good point. But actually now we ca tweak the .pb.cfg file instead of the script itself. Should we file a blocked-by to ensure we don't deprecate this flag?

TO make sure I'm following, FLAGS_quic_reloadable_flag_quic_enable_ack_decimation makes downloads 30% better and uploads 3% worse?
Naive question: if this flag causes a 30% downgrade in upload performance and 30% improvement in download perf, can we enable it just for downloads?
I don't think that's practical, because this is a per-connection feature, and a given connection may be a mix of uploads and downloads.  Experiments in Chrome Stable and the YouTube app have showed it improved YouTube QoE and search latency.

I do plan to get back to this, and I believe I may know how to fix this, but I'm currently blocking multiple other people on other projects due to work I need to finish, so I'm trying to unblock them first.

Sign in to add a comment