Page MenuHomePhabricator

rust-discovery: optimization of add commons/missings for empty arguments
ClosedPublic

Authored by gracinet on May 22 2019, 1:00 PM.

Details

Summary

These two cases have to be catched early for different reasons.
In the case of add_missing_revisions, we don't want to trigger
the computation of the undecided set (and the children cache)
too early: the later the better.

In the case of add_common_revisions, the inner MissingAncestors
object wouldn't know that all ancestors of its bases have already
been removed from the undecided.
In principle, that would in itself be a lead for further
improvement: this remove_ancestors_from could be more incremental,
but the current performance seems to be good enough.

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.May 22 2019, 1:00 PM
kevincox requested changes to this revision.Jun 10 2019, 8:53 AM

Wouldn't it be better to make add_missing_revisions work properly on empty inputs? It seems like it is too error prone to try and catch every caller. If possible I would like to push this check down as far as possible.

It seems easy enough to do in https://www.mercurial-scm.org/repo/hg/file/tip/rust/hg-core/src/discovery.rs#l56 by checking if common grew. This has the added benefit that attempting to re-add bases will also skill the work.

This revision now requires changes to proceed.Jun 10 2019, 8:53 AM
gracinet retitled this revision from rust-discovery: avoid useless calls to addcommons/addmissings to rust-discovery: optimization of add commons/missings for empty arguments.Jun 12 2019, 2:17 PM
gracinet edited the summary of this revision. (Show Details)
gracinet updated this revision to Diff 15472.
gracinet updated this revision to Diff 15486.Jun 13 2019, 9:33 AM

Wouldn't it be better to make add_missing_revisions work properly on empty inputs? It seems like it is too error prone to try and catch every caller. If possible I would like to push this check down as far as possible.
It seems easy enough to do in https://www.mercurial-scm.org/repo/hg/file/tip/rust/hg-core/src/discovery.rs#l56 by checking if common grew. This has the added benefit that attempting to re-add bases will also skill the work.

Done in the update version.

@kevincox, thanks for the review of this and its predecessors. I think I've addressed all your remarks and improvement suggestions.

kevincox accepted this revision.Jun 13 2019, 10:00 AM

Thanks, looks good.