This is an archive of the discontinued Mercurial Phabricator instance.

rust-nodemap: core implementation for shortest
ClosedPublic

Authored by gracinet on Jan 10 2020, 10:04 AM.

Details

Summary

In this implementation, we just make lookup() return also the number
of steps that have been needed to come to a conclusion from the
nodetree data, and validate_candidate() takes care of the special
cases related to NULL_NODE.

This way of doing minimizes code duplication, but it means that
the comparatively slower finding of first non zero nybble will run
for all calls to find() where it is not needed.

Still running on the file generated for the mozilla-central repository,
it seems indeed that we now get more ofter 320 ns than 310. The odds that
this could have a significant impact on real life Mercurial performance
are still looking low. Let's wait for actual benchmark runs to see if
an optimization is needed here.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gracinet created this revision.Jan 10 2020, 10:04 AM
gracinet updated this revision to Diff 19331.Jan 15 2020, 6:48 PM
kevincox accepted this revision.Jan 16 2020, 7:24 AM
kevincox added inline comments.
rust/hg-core/src/revlog/nodemap.rs
431–444

Can you describe the return value in the doc comment.

698

How about unique_bin_prefix_len?

898

unique_bin_prefix_len?

martinvonz added inline comments.
rust/hg-core/src/revlog/nodemap.rs
96

can the explicit lifetime be dropped?

319–328

s/cand/candidate/? I prefer to avoid unusual abbreviations.

gracinet added inline comments.Jan 22 2020, 1:28 PM
rust/hg-core/src/revlog/nodemap.rs
698

I have personally nothing against this, and actually find this shortest not to be clear, but… it's also the name in the C implementation and the Python API, IIRC.

@martinvonz I see you subscribed, what do you think ?

martinvonz added inline comments.Jan 22 2020, 1:39 PM
rust/hg-core/src/revlog/nodemap.rs
698

I'm fine with either. We'll expose it as "shortest" in the python API anyway, I assume. Feel free to change as far as I'm concerned.

gracinet updated this revision to Diff 19639.Jan 27 2020, 10:50 AM
gracinet marked 3 inline comments as done.Jan 27 2020, 11:08 AM
gracinet added inline comments.
rust/hg-core/src/revlog/nodemap.rs
431–444

Done

698

Okay, I've settled with unique_prefix_len_bin etc, keeping the convention that it's something_bin, something_hex, etc.

gracinet marked an inline comment as done.Jan 27 2020, 11:55 AM
gracinet updated this revision to Diff 19640.
marmoute accepted this revision.Feb 7 2020, 3:03 PM
marmoute added a subscriber: marmoute.

It seems like all the feedback have been applied, and the changeset looks good to me.

Alphare updated this revision to Diff 20218.Feb 14 2020, 6:02 AM
Alphare updated this revision to Diff 20237.Feb 15 2020, 6:50 AM
Alphare updated this revision to Diff 20278.Feb 24 2020, 5:13 AM
Alphare updated this revision to Diff 20283.Feb 24 2020, 8:15 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.