This is an archive of the discontinued Mercurial Phabricator instance.

rust-dirstatemap: cache non normal and other parent set
ClosedPublic

Authored by marmoute on Feb 12 2020, 6:32 PM.

Details

Summary

Performance of hg update was significantly worse since the introduction of
the Rust dirstatemap. This regression was noticed by Valentin Gatien-Baron
when working on a large repository, as it goes unnoticed for smaller
repositories like Mercurial itself.

This fix introduces the same getter/setter mechanism at hg-core level as
for set/get_dirs.

While this technique is, as previously discussed, quite suboptimal, it fixes an
important enough problem. Refactoring hg-core to use the typestate
pattern could be a good approach to improving code quality in a future patch.

This is a graft of stable of 83b2b829c94e

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

marmoute created this revision.Feb 12 2020, 6:32 PM

INTENDED FOR STABLE

kevincox added inline comments.Feb 13 2020, 8:15 AM
rust/hg-core/src/dirstate/dirstate_map.rs
38

I don't understand why an Option<HashSet> is faster than a HashSet. Could you add some explanation to the commit message? Is this to avoid attempting to initialize the entry multiple times?

251

If we have just set the fields to Some(..) in the previous line can't we do the unwrap here where it is obviously correct?

This is supposed to be a graft of something already accepted on default. So unless I did a mistake on the graft (cc @Alphare for checking) any feedback on this also apply on the default one.

This is supposed to be a graft of something already accepted on default. So unless I did a mistake on the graft (cc @Alphare for checking) any feedback on this also apply on the default one.

This looks good (in that it looks exactly as bad as it should), thanks for the graft.

Alphare added inline comments.Feb 13 2020, 10:28 AM
rust/hg-core/src/dirstate/dirstate_map.rs
38

Using Option adds a layer of indirection so that any endpoint that needs the set can initialize it without having to doubt if it was already initialized.

251

Unwrapping creates a temporary value that is dropped instantly, so unless I'm not seeing something, I don't think we can.

Alphare accepted this revision.Feb 13 2020, 10:28 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.
kevincox added inline comments.Feb 14 2020, 4:39 AM
rust/hg-core/src/dirstate/dirstate_map.rs
251

You should be able to use .as_mut() just like you are currently doing in the caller.

Alphare added inline comments.Feb 14 2020, 6:31 AM
rust/hg-core/src/dirstate/dirstate_map.rs
251

as_mut() only does &mut Option<T> -> Option<&mut T>, which isn't always what we want (however you can argue that it would save a few keystrokes), but calling unwrap() circles back to the same error[E0515]: cannot return value referencing temporary value.

Alphare added inline comments.Feb 14 2020, 8:54 AM
rust/hg-core/src/dirstate/dirstate_map.rs
251

Aaah I still had &mut, and the auto-deref rules didn't work there. I see that this is much better. I will send a follow-up on default, as that is just code cleanup and does not constitute a bug fix.

Thanks!