Page MenuHomePhabricator

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

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


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

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

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.

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

Diff Detail

rARC Arcanist
Lint Passed
Tests Passed
Build Status
Buildable 20842
Build 28348: Run Core Tests
Build 28347: arc lint + arc unit

Event Timeline

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!

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