Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2516973005:
Update ts_proxy to latest commit (Closed)
|
Created:
4 years, 1 month ago by nednguyen Modified:
4 years ago Reviewers:
Pat Meenan CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org, charliea (OOO until 10-5) Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionUpdate ts_proxy to latest commit
For all the changes, see: https://github.com/WPO-Foundation/tsproxy/compare/6fae6141104d077c8ceb1136b6ddee5b98f30240...9ae7d2ff822cb7cef15bc54ff614273c99691744
The main change is "Reduced CPU impact when idle or running in bypass mode".
Bug: https://code.google.com/p/chrome-os-partner/issues/detail?id=60009
BUG=chromium:653519
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4e71cc39324b205febf5a4cfa3a089a1e2aeab74
Patch Set 1 #Patch Set 2 : up #Patch Set 3 : Sync further #
Messages
Total messages: 27 (15 generated)
Description was changed from ========== Update ts_proxy to latest commit BUG=https://code.google.com/p/chrome-os-partner/issues/detail?id=60009 ========== to ========== Update ts_proxy to latest commit Bug: https://code.google.com/p/chrome-os-partner/issues/detail?id=60009 BUG=chromium:653519 ==========
Description was changed from ========== Update ts_proxy to latest commit Bug: https://code.google.com/p/chrome-os-partner/issues/detail?id=60009 BUG=chromium:653519 ========== to ========== Update ts_proxy to latest commit For all the changes, see: https://github.com/WPO-Foundation/tsproxy/compare/6fae6141104d077c8ceb1136b6d... The main change is "Reduced CPU impact when idle or running in bypass mode". Bug: https://code.google.com/p/chrome-os-partner/issues/detail?id=60009 BUG=chromium:653519 ==========
nednguyen@google.com changed reviewers: + pmeenan@chromium.org
The CQ bit was checked by nednguyen@google.com 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: Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
Are the trybots failing because the commands are taking too long to respond for setting the shaping or is something else going on? Each command could take up to 1 second to get an OK response with the slower tick rate. The only way to make it fast AFAIK would be to use a socket-based interface for sending commands instead of the console so the socket polling loop would detect the commands immediately. I could potentially speed things up a bit by allowing all of the parameters to be specified in a single command if that would help.
On 2016/11/21 22:53:43, nednguyen wrote: Patrick, looks like this change to ts_proxy is making the page takes much longer to load on Mac & Linux for the config (rtt=300, inkbps=32000, outkbps=6400). It's timing out with 30s limit for navigation, hence the test is failing.
On 2016/11/22 13:23:21, nednguyen wrote: > On 2016/11/21 22:53:43, nednguyen wrote: > > Patrick, looks like this change to ts_proxy is making the page takes much longer > to load on Mac & Linux for the config (rtt=300, inkbps=32000, outkbps=6400). > It's timing out with 30s limit for navigation, hence the test is failing. Oops, just seen your comment. To clarify, it's not the response time of the command is at fault here. They run relatively fast on my machine at least. Somehow proxying the network data takes much longer than expected traffic config.
On 2016/11/22 13:25:47, nednguyen wrote: > On 2016/11/22 13:23:21, nednguyen wrote: > > On 2016/11/21 22:53:43, nednguyen wrote: > > > > Patrick, looks like this change to ts_proxy is making the page takes much > longer > > to load on Mac & Linux for the config (rtt=300, inkbps=32000, outkbps=6400). > > It's timing out with 30s limit for navigation, hence the test is failing. > > Oops, just seen your comment. To clarify, it's not the response time of the > command is at fault here. They run relatively fast on my machine at least. > > Somehow proxying the network data takes much longer than expected traffic > config. Ugh. And only on Mac/Linux :-( Setting up a Linux VM now but it will take a while to grab all of the code and build to reproduce locally.
On 2016/11/22 14:20:30, Pat Meenan wrote: > On 2016/11/22 13:25:47, nednguyen wrote: > > On 2016/11/22 13:23:21, nednguyen wrote: > > > On 2016/11/21 22:53:43, nednguyen wrote: > > > > > > Patrick, looks like this change to ts_proxy is making the page takes much > > longer > > > to load on Mac & Linux for the config (rtt=300, inkbps=32000, outkbps=6400). > > > It's timing out with 30s limit for navigation, hence the test is failing. > > > > Oops, just seen your comment. To clarify, it's not the response time of the > > command is at fault here. They run relatively fast on my machine at least. > > > > Somehow proxying the network data takes much longer than expected traffic > > config. > > Ugh. And only on Mac/Linux :-( > > Setting up a Linux VM now but it will take a while to grab all of the code and > build to reproduce locally. Multiplatform support is hard :P...
On 2016/11/22 14:23:42, nednguyen wrote: > On 2016/11/22 14:20:30, Pat Meenan wrote: > > On 2016/11/22 13:25:47, nednguyen wrote: > > > On 2016/11/22 13:23:21, nednguyen wrote: > > > > On 2016/11/21 22:53:43, nednguyen wrote: > > > > > > > > Patrick, looks like this change to ts_proxy is making the page takes much > > > longer > > > > to load on Mac & Linux for the config (rtt=300, inkbps=32000, > outkbps=6400). > > > > It's timing out with 30s limit for navigation, hence the test is failing. > > > > > > Oops, just seen your comment. To clarify, it's not the response time of the > > > command is at fault here. They run relatively fast on my machine at least. > > > > > > Somehow proxying the network data takes much longer than expected traffic > > > config. > > > > Ugh. And only on Mac/Linux :-( > > > > Setting up a Linux VM now but it will take a while to grab all of the code and > > build to reproduce locally. > > Multiplatform support is hard :P... Lets try this again. Can you grab the latest and update? I fixed a few issues with the new direct-passthrough and slow polling logic and it is working for me on my local Linux VM now (and clearing the tests).
The CQ bit was checked by nednguyen@google.com 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...
Description was changed from ========== Update ts_proxy to latest commit For all the changes, see: https://github.com/WPO-Foundation/tsproxy/compare/6fae6141104d077c8ceb1136b6d... The main change is "Reduced CPU impact when idle or running in bypass mode". Bug: https://code.google.com/p/chrome-os-partner/issues/detail?id=60009 BUG=chromium:653519 ========== to ========== Update ts_proxy to latest commit For all the changes, see: https://github.com/WPO-Foundation/tsproxy/compare/6fae6141104d077c8ceb1136b6d... The main change is "Reduced CPU impact when idle or running in bypass mode". Bug: https://code.google.com/p/chrome-os-partner/issues/detail?id=60009 BUG=chromium:653519 ==========
On 2016/11/23 17:05:55, Pat Meenan wrote: > On 2016/11/22 14:23:42, nednguyen wrote: > > On 2016/11/22 14:20:30, Pat Meenan wrote: > > > On 2016/11/22 13:25:47, nednguyen wrote: > > > > On 2016/11/22 13:23:21, nednguyen wrote: > > > > > On 2016/11/21 22:53:43, nednguyen wrote: > > > > > > > > > > Patrick, looks like this change to ts_proxy is making the page takes > much > > > > longer > > > > > to load on Mac & Linux for the config (rtt=300, inkbps=32000, > > outkbps=6400). > > > > > It's timing out with 30s limit for navigation, hence the test is > failing. > > > > > > > > Oops, just seen your comment. To clarify, it's not the response time of > the > > > > command is at fault here. They run relatively fast on my machine at least. > > > > > > > > Somehow proxying the network data takes much longer than expected traffic > > > > config. > > > > > > Ugh. And only on Mac/Linux :-( > > > > > > Setting up a Linux VM now but it will take a while to grab all of the code > and > > > build to reproduce locally. > > > > Multiplatform support is hard :P... > > Lets try this again. Can you grab the latest and update? I fixed a few issues > with the new direct-passthrough and slow polling logic and it is working for me > on my local Linux VM now (and clearing the tests). The failed test is passing locally now! I have high hope
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/23 17:12:08, nednguyen wrote: > On 2016/11/23 17:05:55, Pat Meenan wrote: > > On 2016/11/22 14:23:42, nednguyen wrote: > > > On 2016/11/22 14:20:30, Pat Meenan wrote: > > > > On 2016/11/22 13:25:47, nednguyen wrote: > > > > > On 2016/11/22 13:23:21, nednguyen wrote: > > > > > > On 2016/11/21 22:53:43, nednguyen wrote: > > > > > > > > > > > > Patrick, looks like this change to ts_proxy is making the page takes > > much > > > > > longer > > > > > > to load on Mac & Linux for the config (rtt=300, inkbps=32000, > > > outkbps=6400). > > > > > > It's timing out with 30s limit for navigation, hence the test is > > failing. > > > > > > > > > > Oops, just seen your comment. To clarify, it's not the response time of > > the > > > > > command is at fault here. They run relatively fast on my machine at > least. > > > > > > > > > > Somehow proxying the network data takes much longer than expected > traffic > > > > > config. > > > > > > > > Ugh. And only on Mac/Linux :-( > > > > > > > > Setting up a Linux VM now but it will take a while to grab all of the code > > and > > > > build to reproduce locally. > > > > > > Multiplatform support is hard :P... > > > > Lets try this again. Can you grab the latest and update? I fixed a few > issues > > with the new direct-passthrough and slow polling logic and it is working for > me > > on my local Linux VM now (and clearing the tests). > > The failed test is passing locally now! I have high hope The CQ is green now! Hopefully the catatapult roll will also be green. Do you want to stamp, Pat?
LGTM
The CQ bit was checked by nednguyen@google.com
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": 1479923633389440,
"parent_rev": "982d024dadecb5a6b73041f349dd0032c532967e", "commit_rev":
"4e71cc39324b205febf5a4cfa3a089a1e2aeab74"}
Message was sent while issue was closed.
Description was changed from ========== Update ts_proxy to latest commit For all the changes, see: https://github.com/WPO-Foundation/tsproxy/compare/6fae6141104d077c8ceb1136b6d... The main change is "Reduced CPU impact when idle or running in bypass mode". Bug: https://code.google.com/p/chrome-os-partner/issues/detail?id=60009 BUG=chromium:653519 ========== to ========== Update ts_proxy to latest commit For all the changes, see: https://github.com/WPO-Foundation/tsproxy/compare/6fae6141104d077c8ceb1136b6d... The main change is "Reduced CPU impact when idle or running in bypass mode". Bug: https://code.google.com/p/chrome-os-partner/issues/detail?id=60009 BUG=chromium:653519 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
