1337946 - Expose pushlog revset predicates on hgweb

Status: RESOLVED FIXED

(Developer Services :: Mercurial: Pushlog, defect)

Description

[Axel Hecht [:Pike]]

It'd be nice if we could use the pushlog revset predicates on the web interface.

Not all of them are implemented such that they're not easy to DOS, though, so it might need some care.

https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/pushlog/__init__.py#l574 and friends is what I'm lookin at.

Comment 1

[Gregory Szorc [:gps]]

Restarting what was said in #hg...

revsets need to be marked as "safe" to allow them to be executed on the hgweb interface. The pushlog revsets aren't IIRC.

The pushlog revsets aren't exactly optimized. For example, pushdate() will fetch the entire pushlog database then iterate through and compare dates. It would be *really* nice if we could boil that down to a SQL query that only returned pushlog entries that matched the date expression. However, Mercurial's date expression is somewhat complicated and a function needs to be called to compare it against a time value. So I think the best we can do is run a SQL query to select all pushlog times, filter on that, then return data on changesets within.

It is possible the existing implementation is "good enough" - running locally on the server shows it to take a few hundred milliseconds to a few seconds. We already have revsets taking that long. As long as this isn't getting abused, I think it will be fine to enable as is.

Comment 2

[Gregory Szorc [:gps]]

Found this bug when writing a comment for bug 1370180. It is low hanging fruit. So I'll just implement it.

Comment 3

[Gregory Szorc [:gps]]

Attachment: pushlog: expose pushlog revsets to hgweb (bug 1337946);

Revsets must explicitly opt in to being exposed on hgweb for
performance/security reasons.

When we implemented the pushlog revsets, I'm pretty sure I just
forgot to enable (at least some of) them on hgweb.

Some of the pushlog revsets are a bit expensive to compute.
pushhead(), pushdate(), and pushuser() each query the full
database then iterate on results to do filtering. For a repo
like mozilla-central which has ~32,000 pushes, `hg perfrevset`
says this takes ~1.11s on my i7-6700k. Other built-in and
enabled revsets are already more expensive. So I think this is
tolerable.

Review commit: https://reviewboard.mozilla.org/r/147358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147358/

Comment 4

[Gregory Szorc [:gps]]

Attachment: pushlog: optimize revision resolution (bug 1337946);

Some revsets iterate over the pushlog and emit revisions. Since the
pushlog API returns nodes, they need to convert nodes to integer
revs.

Previously, they were using repo[x].rev() to resolve a node to a rev.
This constructs a full changectx type, which can be expensive. We
rewrite the code to perform node -> rev translations in the most
efficient manner possible.

The impact on revset performance for mozilla-central is clearly visible:

$ hg perfrevset 'pushhead()'
! wall 1.108759 comb 1.110000 user 1.100000 sys 0.010000 (best of 9)
! wall 0.752788 comb 0.750000 user 0.740000 sys 0.010000 (best of 14

$ hg perfrevset 'pushdate(">2017")'
! wall 1.026203 comb 1.020000 user 1.020000 sys 0.000000 (best of 10)
! wall 0.722270 comb 0.720000 user 0.710000 sys 0.010000 (best of 14)

$ hg perfrevset 'pushuser(gszorc)'
! wall 0.788897 comb 0.790000 user 0.790000 sys 0.000000 (best of 13)
! wall 0.716771 comb 0.720000 user 0.700000 sys 0.020000 (best of 14)

Review commit: https://reviewboard.mozilla.org/r/147360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147360/

Comment 5

[:glob ✱]

Comment on attachment 8875958 [details]
pushlog: optimize revision resolution (bug 1337946);

https://reviewboard.mozilla.org/r/147360/#review154268

::: hgext/pushlog/__init__.py:594
(Diff revision 1)
>      # involved. However, this is less overhead than issuing a SQL query for
>      # every changeset, especially on large repositories. There is room to make
>      # this optimal by batching SQL, but that adds complexity. For now,
>      # simplicity wins.
>      def getrevs():
> +        torev = repo.changelog.rev

for clarity please name this `to_rev`

Comment 6

[:glob ✱]

Comment on attachment 8875957 [details]
pushlog: expose pushlog revsets to hgweb (bug 1337946);

https://reviewboard.mozilla.org/r/147358/#review154270

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8875957 [details]
pushlog: expose pushlog revsets to hgweb (bug 1337946);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147358/diff/1-2/

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8875958 [details]
pushlog: optimize revision resolution (bug 1337946);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147360/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/5bc8e4314613
pushlog: expose pushlog revsets to hgweb ; r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/3986d9ddbc08
pushlog: optimize revision resolution ; r=glob

Comment 10

[Gregory Szorc [:gps]]

FWIW, you can convert from bin node to rev even faster by going through repo.changelog.nodemap. I learned that from seeing a patch on the hg dev list authored after I submitted this review. It only buys us 20-30ms on 750ms. I don't think it is worth it at this juncture.

Comment 11

[Gregory Szorc [:gps]]

This is deployed. However, the revsets aren't working in the search box on hg.mozilla.org. I think this is a bug in the WSGI server not running extra code at extension load time that command dispatch does.

Comment 12

[Gregory Szorc [:gps]]

https://bz.mercurial-scm.org/show_bug.cgi?id=5601 filed.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/fd93826ec793
pushlog: work around Mercurial bug to expose revsets to hgweb