1671351 - remove all calls to hg via subprocess

Status: RESOLVED FIXED

(Conduit :: moz-phab, enhancement)

Description

[Tarek Ziadé (:tarek)]

moz-phab is calling hg cat and hg files on EACH file it processes. This is extremely expensive, you need to start a python process everytime.

Let's use directly the mercurial lib.

ALL calls to hg should be migrated to use the lib.

I'll start to do it in https://github.com/tarekziade/review - feel free to use that clone to try out (speedup branch)

Comment 1

[Julien Cristau [:jcristau]]

using mercurial's internals directly sounds misguided, there's no API compat guarantees

Comment 2

[Sylvestre Ledru [:Sylvestre]]

I am not saying that it is good but we have significant precedents in our tooling. For example:
https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/clang-format/__init__.py
(and many other files in this repo)

Comment 3

[Julien Cristau [:jcristau]]

Sure, but these are hg extensions so you kind of don't have a choice there. There's ways to avoid forking hg many times from moz-phab without going the "use mercurial internals" way (off the top of my head, using hglib, or chg, but even without that, avoiding repeating the same things over and over, as documented in other bugs hanging off of 1649086), since that is likely to just increase your maintenance cost. And you'll end up just reinventing hg's own phabricator extension, which also seems like wasted effort. Obviously it's ultimately not my call.

Comment 4

[:glob ✱]

(In reply to Julien Cristau [:jcristau] from comment #1)

using mercurial's internals directly sounds misguided, there's no API compat guarantees

+1
See https://www.mercurial-scm.org/wiki/MercurialApi#Why_you_shouldn.27t_use_Mercurial.27s_internal_API
There's also licensing concerns as linking with mercurial internals forces us to use GPL.

Using https://www.mercurial-scm.org/wiki/PythonHglib will avoid creating a process for every hg query, but using a stable API.

Comment 5

[Tarek Ziadé (:tarek)]

Thanks all, I moved to python-hlib and we're seeing the same gains. (3-4x)

I've updated my PR and will work on the tests to make sure everything's fine. If you want to give it a shot, please do!

Comment 6

[Tarek Ziadé (:tarek)]

Attachment: Bug 1671351 - replace subprocess calls by hglib calls r?glob

Comment 8

[Tarek Ziadé (:tarek)]

Attachment: Bug 1671351 - remove all calls to hg via subprocess r?glob

Comment 9

[:glob ✱]

https://github.com/mozilla-conduit/review/commit/bc07a811ecbd3b645cbe994db32a4e1f2f6006e6

Comment 10

[:glob ✱]

This failed the Windows tests. I'll investigate and fix.

Comment 11

[:glob ✱]

https://github.com/mozilla-conduit/review/commit/2325ac1e1009cfffc834526c9072757fae598cb0