Page MenuHomePhabricator

Don't pass `revisionIDs` to `differential.querydiffs`
AbandonedPublicDraft

Authored by joshuaspence on Sep 17 2018, 4:57 AM.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T11699: `arc patch` is timing out
Summary

Ref T11699. Currently, arc patch D123 will make a Conduit request to differential.querydiffs with parameters ['revisionIDs' => ['D123']]. This ends up loading all DifferentialDiffs associated with a DifferentialRevision, as well as the changeset for each diff. Arcanist then discards most of the response by calling head($future->resolve()).

We had users reported that arc patch was failing for one particular Differential revision:

[2018-09-16 23:47:57] EXCEPTION: (HTTPFutureCURLResponseStatus) [cURL/28] (https://phabricator.REDACTED/api/differential.querydiffs) <CURLE_OPERATION_TIMEDOUT> The request took too long to complete. at [<phutil>/src/future/http/HTTPSFuture.php:408]
arcanist(head=master, ref.master=30b7835c37b5), flarc(), phutil(head=master, ref.master=a28f6e5d64f3)
  #0 HTTPSFuture::isReady() called at [<phutil>/src/future/Future.php:37]
  #1 Future::resolve(NULL) called at [<phutil>/src/future/FutureProxy.php:34]
  #2 FutureProxy::resolve() called at [<arcanist>/src/workflow/ArcanistWorkflow.php:1195]
  #3 ArcanistWorkflow::loadBundleFromConduit(ConduitClient, array) called at [<arcanist>/src/workflow/ArcanistWorkflow.php:1186]
  #4 ArcanistWorkflow::loadRevisionBundleFromConduit(ConduitClient, string) called at [<arcanist>/src/workflow/ArcanistPatchWorkflow.php:377]
  #5 ArcanistPatchWorkflow::run() called at [<arcanist>/src/workflow/ArcanistPatchWorkflow.php:390]
  #6 ArcanistPatchWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]

I personally couldn't reproduce this issue, but I did notice that the response from differential.querydiffs was ridiculously large:

> echo '{"revisionIDs": ["105748"]}' | arc call-conduit differential.querydiffs | wc --bytes
14278157

Instead of modifying differential.querydiffs (which is frozen), we can improve the performance of arc patch by looking up the most-recent DifferentialDiff for the specified DifferentialRevision and calling differential.querydiffs with ids instead of revisionIDs.

Test Plan

time arc patch ... before and after this change.

Before
real	0m15.407s
user	0m1.208s
sys	0m0.696s
After
real	0m3.332s
user	0m0.584s
sys	0m0.500s

Diff Detail

Repository
rARC Arcanist
Branch
master
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 20842
Build 28348: Run Core Tests
Build 28347: arc lint + arc unit

Event Timeline

joshuaspence created this revision.Sep 17 2018, 4:57 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Sep 17 2018, 4:57 AM
Harbormaster failed remote builds in B20842: Diff 47031!

Build failure is unrelated.

joshuaspence abandoned this revision.Mar 1 2019, 12:59 AM

Superseded by D20221.

This is probably complimentary to D20221, I think? That one's on arc diff and this one's in arc patch?