Issue metadata
Sign in to add a comment
|
3.6% regression in cronet_perf_tests at 558091:558095 |
||||||||||||||||||||
Issue descriptionhttps://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?
,
May 14 2018
,
May 14 2018
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?
,
May 14 2018
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
,
May 15 2018
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)
,
May 15 2018
+fayang@: Fan, could you take a close look on those flags and spot which might be the related flag? Thanks!
,
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
,
May 16 2018
The commit in #7 significantly (+31.2%) reduced the QUIC download performance. See: https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICQwYqn_QgM
,
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
,
May 16 2018
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.
,
May 16 2018
https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICQ_pqfpQsM Looks like the spotted flag is: FLAGS_quic_reloadable_flag_quic_enable_ack_decimation.
,
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).
,
May 16 2018
,
May 16 2018
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!
,
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?
,
Jun 19 2018
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?
,
Jun 19 2018
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 |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, May 14 2018