( )⚙ D37 phabricator: avoid calling differential.getrawdiff

This is an archive of the discontinued Mercurial Phabricator instance.

phabricator: avoid calling differential.getrawdiff
AbandonedPublic

Authored by quark on Jul 11 2017, 11:56 AM.

Details

Reviewers
durin42
Summary

Previously, we call differential.getrawdiff per patch despite we have the
diff content already (returned by differential.querydiffs).

This patch implements the serialization logic of differential.getrawdiff
client-side so we no longer need calling differential.getrawdiff. This
removes one API call per patch.

Now fetching a stack of 10 patches is just 2 API calls in the best case.
Worst case batching does not work and it's 11 API calls. The "reading"
verbose message was removed since there is no remote IO needed to read a
patch in that loop now.

Using a test Phabricator instance, this patch reduces phabread reading a
10-patch stack from about 8 seconds to 1.3 seconds. Even with batching
disabled, reading that 10-patch stack could be done in 4 seconds. The
copy-paste constants are a bit unfortunate since "code is documentation" is
true for many Phabricator features. But the perf win could probably justify
it.

Test Plan

Create an interesting (contains multiple copies and renames, mixed mode
change with content changes) patch using the below script:

hg init

# long context could be more interesting
seq 1 200 > long
for i in a b c d e f g; do
  echo $i > $i
done
hg add .
hg commit -m init

sed -i '/10/d;s/22$/22.1\n22.2/g' long

# multiple copies and moves
for i in a b; do
  for j in 1 2 3 4; do
    hg cp $i "${i}${j}"
    [ $((j % 2)) = 0 ] && chmod +x "${i}${j}"
    [ $j -ge 3 ] && echo append >> "${i}${j}"
  done
done

hg rm a
chmod +x c
hg rm d
hg mv e E
chmod +x E
rm f
touch f
hg commit -m change

Send it using phabsend, and retrieve it using phabread. Compare before
and after this patch. The only differences are:

  • This patch removes a blank line after a patch.
  • This patch does not omit ,1 in diff hunks. See [1].

Other than that, the patch output is exactly the same. So the new code path
looks reasonably good.

Also try using phabread to read all existing diffs sent to the website and
make sure it works.

[1]: https://secure.phabricator.com/diffusion/ARC/browse/master/src/parser/
ArcanistBundle.php;e00e2939c164df85852ce2d157de4197b399c30b$631

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Jul 11 2017, 11:56 AM
quark edited the test plan for this revision. (Show Details)Jul 11 2017, 12:22 PM
durin42 edited edge metadata.Jul 14 2017, 11:56 AM

I'm honestly not comfortable with this, after reflection. Phabricator should document their constants, and honestly it feels really nasty to re-synthesize diffs when phabricator can already hand them to us.

If the time to fetch a stack is not a concern, I can drop this.

In D37#1094, @quark wrote:

If the time to fetch a stack is not a concern, I can drop this.

I'm not worried about it for now. Speed of hg phabread is nowhere near even being a pain point for me (the web UI is a much bigger issue)

quark abandoned this revision.Jul 14 2017, 4:34 PM