1494697 - moz-phab is slow to submit large stacks

Status: RESOLVED FIXED

(Conduit :: moz-phab, defect, P2)

Description

[:glob ✱]

moz-phab is slow to submit large stacks

spawned from bug 1491267

Comment 1

[:glob ✱]

from https://bugzilla.mozilla.org/show_bug.cgi?id=1491267#c10 gps points out that watchman isn't used.

moz-phab runs with a clean hgrc, selectively enabling extensions to ensure submissions happen within a more controlled environment.


we could change this approach, and by default run with the user's full hgrc (with an escape hatch to run with our sanitised set), and/or add watchman to the extensions we use.

Comment 2

[Karl Tomlinson (:karlt)]

To be clear, the rebases are O(n^2) with evolve installed.  It is as if "hg evolve --all" is run after each commit description is updated.

Comment 3

[:glob ✱]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> To be clear, the rebases are O(n^2) with evolve installed.  It is as if "hg
> evolve --all" is run after each commit description is updated.

`hg rebase` is _not_ executed when evolve is installed (nor is `hg evolve --all`).

Comment 4

[:glob ✱]

i've spun comment 1 out into bug 1495777

Comment 5

[Karl Tomlinson (:karlt)]

I wonder what "Rebase children atop new commit node" means then.
https://github.com/mozilla-conduit/review/blob/97bb2b95868ee1e15d9f949bccf278ccb95f5e4a/moz-phab#L792

Comment 6

[:glob ✱]

oh! it means i was completely wrong about not performing rebases.

sorry about that; i have no idea how i missed that (and i wrote that code) :|



moving forward zalun's going to perform timing tests of moz-phab against m-c to identify exactly where the pain points are.

Comment 7

[Karl Tomlinson (:karlt)]

Hey, the comments were helpful to someone looking through the code, thanks :)

Comment 8

[Tim Nguyen :ntim]

I wonder if we can have a mode where moz-phab just aborts submission if it needs to rebase. Something like:

"You need to rebase your commits in order to submit to Phabricator. Please run `...` to fix this problem."


This way, it's up to developer to decide how they want to rebase.

Comment 9

[Botond Ballo [:botond]]

Besides performance, another consideration is working copy churn. Working copy churn can trigger rebuilds which are slow (mitigated by ccache, but often not very effectively).

My recollection is that with MozReview, if you had a patch series that touches a commonly included file (like nsLayoutUtils.h), pushing that patch series did not involve touching the file in the working copy in a way that causes the build system to pick it up as changed. It would be very nice if Phabricator preserved that property.

Comment 10

[:glob ✱]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #8)
> I wonder if we can have a mode where moz-phab just aborts submission if it
> needs to rebase.

unfortunately i don't think that's viable.  the rebase isn't being performed to update your commits to the latest head or similar, it's a required step due to how evolve works.


after submission a commit's description needs to be updated to add the phabricator revision url, to ensure that subsequent updates to that commit update the existing revision instead of always creating a new revision.  moz-phab will also update the bug id and reviewers to reflect the command line options, but that isn't strictly required.

the problem is if a commit has children; when its updated by evolve (ie. marked as obsolete) its children are orphaned and need to be rebased onto the new commit that was created with the updated description.
 


i suspect bug 1495777 will go a long way to helping performance here, as currently moz-phab never enables watchman, but it's hard to say for sure without seeing logs with timings.  anyone could collect those logs by running `DEBUG=1 moz-phab submit ... | ts` (requires `ts` from the `moreutils` package).


(In reply to Botond Ballo [:botond] from comment #9)
> My recollection is that with MozReview, if you had a patch series that
> touches a commonly included file (like nsLayoutUtils.h), pushing that patch
> series did not involve touching the file in the working copy in a way that
> causes the build system to pick it up as changed. It would be very nice if
> Phabricator preserved that property.

if you're stating that phabricator incorrectly touches files during submission please file a new bug for that.

Comment 11

[:glob ✱]

Attachment: moz-phab-stack-default.txt

trace of 10 commit stack, with just evolve

Comment 12

[:glob ✱]

Attachment: moz-phab-stack-fsmon.txt

trace of 10 commit stack, with evolve and fsmonitor

Comment 13

[:glob ✱]

timings of the two runs attached is pretty well the same, at 8 minutes.

Comment 14

[:glob ✱]

Attachment: long running commands (>= 5 seconds)

Comment 15

[:glob ✱]

i'm currently exploring a wide range of options with regards to improving the performance.

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: 18 commits / 18 minutes DEBUG=1 moz-phab submit | ts

Comment 17

[Gregory Szorc [:gps]]

I see lines like the following:

Oct 30 18:53:23 DEBUG    2018-10-30 18:53:23,725 $ arc --trace diff --base arc:this --allow-untracked --no-amend --no-ansi --message-file /tmp/tmpaMhuMS --create
Oct 30 18:53:36  ARGV  '/var/karl/arcanist/bin/../scripts/arcanist.php' '--trace' 'diff' '--base' 'arc:this' '--allow-untracked' '--no-amend' '--no-ansi' '--message-file' '/tmp/tmpaMhuMS' '--create'
Oct 30 18:53:36  LOAD  Loaded "phutil" from "/var/karl/libphutil/src".
Oct 30 18:53:36  LOAD  Loaded "arcanist" from "/var/karl/arcanist/src".

It kinda/sorta looks like it is taking 13s to start/run arc?!

Comment 18

[Karl Tomlinson (:karlt)]

Thanks for having a look.  The following lines make it look like that arc command took no time to create the differential revision, but I suspect the unexpected times to due to arc output being buffered.

Oct 30 18:53:36 Linting...
Oct 30 18:53:36 No lint engine configured for this project.
Oct 30 18:53:36 Running unit tests...
Oct 30 18:53:36 No unit test engine is configured for this project.
Oct 30 18:53:36 Created a new Differential revision:
Oct 30 18:53:36         Revision URI: https://phabricator.services.mozilla.com/D10159
Oct 30 18:53:36 
Oct 30 18:53:36 Included changes:
Oct 30 18:53:36   M       dom/media/MediaStreamGraph.h
Oct 30 18:53:36 DEBUG    2018-10-30 18:53:36,837 $ hg --config 'ui.username=Karl Tomlinson <[e-mail]>' --config extensions.rebase= --config extensions.evolve= --config extensions.mq= log -T '{desc}' -r 2315c13950cd28ba61cc311b59cde287bf306416

Comment 19

[Piotr Zalewa [:zalun]]

One of the reasons of git support being slow is running a `rev-list` on every commit. We should run it only once for the oldest commit in the stack. I'm preparing a patch right now.

Comment 21

[:glob ✱]

here's the plan to address performance issues: https://docs.google.com/document/d/14v33dMrAvs9KlhYVdbN7yYqJpubs8XFpnrDlsZTPBW0/

there's multiple points that can be worked on in parallel:
- reduce the number of rebases
- stop wrapping arc completely; hit the conduit APIs directly
- stop amending commits; use local-only IDs injected at commit time to map to revisions

Comment 22

[Sylvestre Ledru [:Sylvestre]]

I don't know if you change anything but pushing this large patch https://phabricator.services.mozilla.com/D12251 was very quick.

Comment 23

[Botond Ballo [:botond]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> I don't know if you change anything but pushing this large patch
> https://phabricator.services.mozilla.com/D12251 was very quick.

It's not about the number of files a patch touches, it's about the number of patches being submitted at once.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Well, while it's still very slow at times (3 minutes for a 4 lines patch earlier today) I haven't had any moz-phab submit lasting more than 10 minutes recently, so I call this an improvement :)

Comment 25

[:glob ✱]

Closing - we've made a lot of progress with regards to moz-phab's performance on large stacks.