Page MenuHomePhabricator

phases: make phase list dense or dictionaries
AbandonedPublic

Authored by joerg.sonnenberger on Jun 23 2020, 8:26 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

When the internal and archived phase was added, allphases became sparse
populated. This dramatically increased the number of lookup operations
for public revisions when they hit phasecache.phase as it it is
iterationing over all phases in the phasesets. Fix this by making the
various data structures either sparse lists or dictionaries, depending
on the purpose. Push the node->revision mapping down into the C module,
no need to do it in Python when it is as much work in the backend.
Document the inconsistency of phaseroots and how it breaks the parent
lookup shortcut.

Overall, this improves a no-change "hg up" for my NetBSD test
repository from 4.7s to 1.3s.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

mjacob added a subscriber: mjacob.Jun 23 2020, 9:23 PM

This is probably not the conceptual feedback you were looking for. I hope it’s still helpful.

mercurial/exchange.py
1031–1032

nit: you could directly iterate on the values

mercurial/exchangev2.py
85

This should probably use itervalues(), but see below.

89–90

If I understand correctly, phases.phasenumber is the same as the key for phase in this for loop. If you use iteritems() above, I think phases.phasenumber is unused.

361

This should probably use itervalues() (not that it makes a difference for the small dict).

mercurial/phases.py
133–137

Is it for backwards compatibility? It wasn’t obvious for me without a comment.

203–204

This line is now outdated.

mercurial/repoview.py
156–157

You could let trackedphaseroots() return only the values directly.

mercurial/exchangev2.py
85

phasenames contains a small number of strings, the copy/reference doesn't matter here.

89–90

Right, possible. This was meant to preserve the original logic for now, but I'll update it for the next iteration.

361

Yeah, I don't think it makes a difference at all and it is more verbose.

mercurial/phases.py
133–137

Yes, both phases can be seen in existing repositories, so the values have to be preserved. Otherwise I would have moved them to 3 and 4, which would have greatly simplified things.

joerg.sonnenberger retitled this revision from phases: make phase list dense or dictionaries [PoC] to phases: make phase list dense or dictionaries.Jun 27 2020, 6:02 PM
joerg.sonnenberger edited the summary of this revision. (Show Details)
joerg.sonnenberger updated this revision to Diff 21721.

Further analysis shows that the main issue here is that the topic exchange triggers a rebuild of the "served-topic" branchmap without a real reason. That doesn't mean the change isn't useful in general.

joerg.sonnenberger marked 6 inline comments as done and an inline comment as not done.Jul 2 2020, 2:19 PM

Addressed most of the issues raised, some of them were considered and rejected (re items()).

The evolve follow-up patch can be found in https://foss.heptapod.net/mercurial/evolve/-/commit/17d9e81355294320beef2ae11435485716592146

mercurial/exchangev2.py
85

Simplified to use items(), but I still don't care about the copying here as the dictionary is small.

pulkit added a subscriber: pulkit.Jul 7 2020, 2:00 PM

It's bit hard to review the patch since it seems that it consists multiple changes. I can see following ones:

  1. Change allphases to not contain unrequired phases
  2. Introduce utility function to check public roots
  3. Conversion of some list to dictionary

There might be more.

Can you kindly split them into individual patches?

mercurial/phases.py
131

I think we can keep these two constants and hence leave how internal and archived are computed.

149

no need of this change since allphases won't contain extra phases now. In future, we will only need to update allphases.

I can split the patch in some smaller parts. Is the API change itself as goal reasonable?

mercurial/phases.py
149

This change makes it IMO clear what localhiddenphases actually is. I was actually considering spelling them all out explicitly, simply to remove the magic. The HIDEABLE_FLAG doesn't really add value.

joerg.sonnenberger abandoned this revision.Jul 7 2020, 6:39 PM

Superseded by D8694, D8695, D8697 and D8698.