( )⚙ D657 revset: move weight information to predicate

This is an archive of the discontinued Mercurial Phabricator instance.

revset: move weight information to predicate
ClosedPublic

Authored by quark on Sep 7 2017, 12:16 PM.

Details

Summary

Previously revset weight is hardcoded and cannot be modified. This patch
moves it to predicate so newly registered revsets could define their weight
to properly give static optimization some hint.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Sep 7 2017, 12:16 PM
phillco accepted this revision.Sep 7 2017, 1:37 PM
phillco added a subscriber: phillco.

lgtm

mercurial/revset.py
256

I take it this dictionary was moved there because of the import direction?

quark added inline comments.Sep 7 2017, 6:15 PM
mercurial/revset.py
256

Yes.

yuja added a subscriber: yuja.Sep 8 2017, 10:08 AM

Perhaps we'll need to make these magic numbers less obfuscated. Before, weights
were defined in one location and functions were grouped, so it wasn't hard to guess
what sort of functions should belonged to e.g. the w=30 group. With this patch,
weights can be arbitrary chosen by extension authors, but could we choose the
right value?

quark added a comment.Sep 8 2017, 2:11 PM

How about trying to document it? ex.

'weight' is an estimated number for static optimization so things like "given
'x & y', calculate x first or y first?" could be decided. 'weight' has a
default value 10. revset with O(changelog.d) complexity usually have weight
100, revset which also needs reading manifest diffs usually have weight 30,
revset reading file contents usually have weight 100. For a similar time
complexity, revset returning less revisions could have less weight.

Constants could pobably help make it more self-documenting.

krbullock requested changes to this revision.Sep 18 2017, 4:13 PM
krbullock added a subscriber: krbullock.

I think either expanding the docs or using constants would be fine, but the default weight looks like it's 1, not 10.

This revision now requires changes to proceed.Sep 18 2017, 4:13 PM
quark added a comment.Sep 18 2017, 7:39 PM

I think it's hard to use constants since that would implicitly limit the possibilities. Time complexity choices are unbounded in theory. So I'll go the documentation approach.

quark updated this revision to Diff 1882.Sep 18 2017, 7:47 PM
quark updated this revision to Diff 1883.
singhsrb accepted this revision.Sep 18 2017, 8:54 PM
yuja requested changes to this revision.Sep 19 2017, 11:04 AM

(copied from the email thread, though I'm okay with the latest patch.)

On Fri, 8 Sep 2017 18:24:44 +0000, phillco (Phil Cohen) wrote:

phillco added a comment.

Constants could pobably help make it more self-documenting.

Perhaps. And maybe we can add a debug command that shows a list of revset
functions sorted by weight.

mercurial/registrar.py
175

contains() does not read file contents, but manifest contents.

This revision now requires changes to proceed.Sep 19 2017, 11:04 AM
quark updated this revision to Diff 1895.Sep 19 2017, 1:31 PM
quark updated this revision to Diff 1896.Sep 19 2017, 1:45 PM
yuja accepted this revision.Sep 20 2017, 8:35 AM

Queued, thanks.

This revision was automatically updated to reflect the committed changes.