Page MenuHomePhabricator

clone: make sure we warm the cache after a clone
ClosedPublic

Authored by marmoute on Jan 15 2021, 10:52 AM.

Details

Summary

This work around any deviciency/limitation of the clone process. In our case
this ensure the persistent nodemap exist with valid content.

Ideally, the cloning process would also do "the right thing". However since
older server will never be able to do "the right thing". The local workaround
will be necessary anyway.

I am not worried by the performance impact of this as hg clone is non-instant
on large repositories where is could matters. Warming the cache if they are
already correct is very fast. And if they are not already warm, this seems like
a good time to do so.

This impact various test as more cache are now warmed sooner, all the change
should be harmless.

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.Jan 15 2021, 10:52 AM

This is a recreation of D9735 after it was dropped by @martinvonz for undocumented reasons.

This is a recreation of D9735 after it was dropped by @martinvonz for undocumented reasons.

It was document in the #mercurial IRC channel. Sorry, I forgot that you prefer email. The reason was that the patch broke tests when you have sqlite installed.

This is a recreation of D9735 after it was dropped by @martinvonz for undocumented reasons.

It was document in the #mercurial IRC channel.

I had a simple ping that "something was dropped". Tha message contains none of the basic information I would expect with a drop:

  • which diff was dropped,
  • why it was dropped,
  • how to reproduce the problem.

I scrolled the backlog a bit a could not find those information. Please don't do this.

The reason was that the patch broke tests when you have sqlite installed.

It turns out three simple lines were missing and that Joerg provided a patch. Please consider folding such patch in the change in the future to save some time for everybody.

Current head of default has a broken test-persistent-nodemap.t because that part is missing.

mharbison72 accepted this revision.Jan 16 2021, 9:26 PM
This revision is now accepted and ready to land.Jan 16 2021, 9:26 PM
This revision was automatically updated to reflect the committed changes.

I'm -0 on this change. Caches are caches and IMO should only be populated on demand.

My specific complaint is I worry about performance implications here. There are likely automated processes (e.g. in CI) that clone a repo and don't perform any operation that touches the cache. Populating the cache could introduce non-trivial, avoidable overhead.

Was any performance testing performed on this patch?

tests/test-tags.t
735–744

The test description is now inconsistent with reality. Please update the description and consider deleting redundant tests.

I'm -0 on this change. Caches are caches and IMO should only be populated on demand.

Too lazy cache has an history of slowing down (sometime massively) reader that have not write access (a common setup server side). We have been consistently steering away from it for a while.