This is an archive of the discontinued Mercurial Phabricator instance.

remotefilelog: create delay for background prefetches
ClosedPublic

Authored by ms2316 on Sep 14 2017, 12:39 PM.
Tags
None
Subscribers
None

Details

Summary

This functionality allows to add delays between background prefetches after
operations that change the working copy parent. By default background
prefetches will be run no often than every 2 minutes, but this is configurable.
This allows to reduce the load on CPU(because prefetch is followed by repack).

Test Plan

Tested manually on fbsource repo by running hg co bookmark several
times and ensuring that only 1 background prefetch is running if 2 minute
timeout has not expired.

Diff Detail

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

Event Timeline

ms2316 created this revision.Sep 14 2017, 12:39 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 14 2017, 12:39 PM
durham requested changes to this revision.Sep 14 2017, 8:15 PM
durham added inline comments.
remotefilelog/__init__.py
793

If you want a path like: "/var/durham/myrepo/.hg/lastprefetch" you should use repo.vfs.join('lastprefetch')

797

What race condition? I didn't think opening a file affected the mtime you'd get below?

800

Do we need to handle the case where we don't have filesystem permission to open the file (above) or write the file (here).

811

I'd rename 'timeout' to 'isready' or something. timeout being True feels like it should mean something bad, when in this case it means something good and that the function should proceed.

This revision now requires changes to proceed.Sep 14 2017, 8:15 PM
ms2316 marked 3 inline comments as done.Sep 15 2017, 9:07 AM
ms2316 added inline comments.
remotefilelog/__init__.py
797

Opening a file does not affect the mtime. However, there might be 2 processes trying to update the mtime at the same time and we want to avoid that. Therefore the construct is here to avoid that.

800

I assume that now if with construct fails(e.g. no permission) we will simply return false

ms2316 marked an inline comment as done.Sep 15 2017, 9:12 AM
ms2316 updated this revision to Diff 1841.

Updated variable names and the way to get a path to .hg of the current repo

simonfar accepted this revision.Sep 15 2017, 9:14 AM

Accepting because Mihails is near end of internship - I can do follow-up diffs if there's something missing.

durham requested changes to this revision.Sep 15 2017, 12:10 PM

I think my comment on the file system permissions still holds and needs to be fixed, otherwise it could cause exceptions in random mercurial commands on readonly file systems.

remotefilelog/__init__.py
797

I still don't understand. Why is two processes updating the mtime bad? Also, how does the open prevent that from happening? Can't two processes open the file at once?

800

'with' just handles the lifetime of the open'd file descriptor. If 'open' throws an exception, the with just propagates it back up. So I think we still need to handle it.

This revision now requires changes to proceed.Sep 15 2017, 12:10 PM
This revision was automatically updated to reflect the committed changes.