1498171 - arcanist takes a long time to submit a patch that includes `mach vendor rust`'s changes

Status: RESOLVED DUPLICATE of bug 1471687

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

Description

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

Attachment: Log

Same patch as in bug 1498144. I gave up after 30 minutes of inactivity from arcanist. This ia 3.7 Mb patch, resulting from a `./mach vendor rust` update.

Attaching logs.

Comment 1

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

Attachment: Looong log

Actually, it turns out that it wasn't entirely dead.

Rerunning the same command with `--trace` shows a few interesting things:

1. ~45 minutes spent marking 16 files as binary;
2. It seems to be uploading lots of data. For a 3.7 Mb patch, if I read the logs correctly, it has so far uploaded ~6Mb of data.
3. It has now been running for ~90 minutes and shows no sign of stopping.

Comment 2

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

Is there is something else I can log for this bug report?

Comment 3

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

Ok, after 3h30 on a fast connection, it seems to have finished uploading.

Comment 4

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

Attachment: Full log

Comment 6

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

An incomplete solution might to output more information about the current status. Calling --trace was my only way to find out it was still advancing.

Comment 7

[:glob ✱]

this ended up as https://phabricator.services.mozilla.com/D8380

the diff is too large for phabricator to package as a raw diff for download

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Tangential, but we should ask the sha2 crate owner to exclude the test data from the published crate.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

.. although the test data is tiny so it doesn't really matter. Ignore me.

Comment 10

[Nathan Froyd [:froydnj]]

This should block requiring phabricator for reviews. WDYT?

Comment 11

[:glob ✱]

some diagnosis to feed into our discussion/decision:

D8380 is a large change - it looks like 24 packages were changed in the Cargo.lock file (97 lines), resulting in 232 files being updated.

1. ~45 minutes spent marking 16 files as binary

the logs have

Do you want to mark these files as binary and continue? [Y/n]

prior to bug 1522391 this would have hung moz-phab waiting for confirmation.

there's another earlier confirmation in the log regarding reusing an old commit description; that also would have caused moz-phab to hang.

2. It seems to be uploading lots of data. For a 3.7 Mb patch, if I read the logs correctly, it has so far uploaded ~6Mb of data.

how phabricator's file context works is it uploads the full version of each file, even if only one line in the file has changed.

bug 1517247 comment 2 talks about detecting this and offering to switch to a context-less upload.

3. It has now been running for ~90 minutes and shows no sign of stopping.
   Ok, after 3h30 on a fast connection, it seems to have finished uploading.

this is concerning however as the version of moz-phab that generated the logs doesn't have timestamps it's hard to know exactly where the slowdown was (timestamps are now part of the debug output).

Comment 12

[:glob ✱]

the answer to the core question - should this issue block the switch to phabricator - is no. we'll continue as planned, but focus on improving submission performance on several fronts.

• the policy around reviewing of vendored code is still in flux. i've been chatting with several people to determine the best approach here that balances security and the viability of reviewing massive changes. for mach vendor rust specifically the advice for now is to break the change into smaller parts (e.g. update Cargo.lock one-by-one). as long as these changes are part of the same stack they will land as part of the same push.

• dealing with binary files is a problem - bug 1521980. we're working through our options there in the short term while we still rely on arc

• upload performance of massive patches is something that upstream have sent strong signals that it's an issue they will not focus on. our plan here is to stop using arc completely. this will give us much more control over what is uploaded and how. this work is expected to be completed before the end of Q2.

Comment 13

[Karl Tomlinson (:karlt)]

IIUC the fundamental reason why these kind of issues need not block bug 1498171 is

If you’re doing something unusual that truly can’t be done with Phabricator, you can of course still attach a file to Bugzilla, and use a needinfo or IRC ping to get sign-off from the reviewer.

(In reply to Byron Jones ‹:glob› 🎈 from comment #12)

for mach vendor rust specifically the advice for now is to break the change into smaller parts (e.g. update Cargo.lock one-by-one). as long as these changes are part of the same stack they will land as part of the same push.

Just to be sure no-one reads too much into this advice, breaking up changes is only appropriate if/when the project builds with each single change. i.e. Each revision should be intended to build successfully.

Comment 14

[:glob ✱]

the core fix for this issue is to stop relying on arc. duping to bug 1471687.