This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5308

store: don't pass 'atomictemp=True' while appending to fncache
Needs RevisionPublic

Authored by pulkit on Nov 27 2018, 8:22 AM.

Details

Reviewers
baymax
Group Reviewers
hg-reviewers
Summary

Appending to an atomictemp file means that the entire file is copied first.
Thanks to Yuya for suggesting.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Nov 27 2018, 8:22 AM
yuja added a subscriber: yuja.Nov 27 2018, 8:37 AM
  • a/mercurial/store.py

+++ b/mercurial/store.py
@@ -486,7 +486,7 @@

if self.addls:
    # if we have just new entries, let's append them to the fncache
    tr.addbackup('fncache')
  • fp = self.vfs('fncache', mode='ab', atomictemp=True)

+ fp = self.vfs('fncache', mode='ab')

Ah, no. addbackup() creates hardlink, which means fncache can't be updated in
place. Also, the reader wouldn't handle partially-written fncache well.

pulkit added a comment.Nov 27 2018, 8:53 AM
In D5308#78985, @yuja wrote:
  • a/mercurial/store.py

+++ b/mercurial/store.py
@@ -486,7 +486,7 @@

if self.addls:
    # if we have just new entries, let's append them to the fncache
    tr.addbackup('fncache')
  • fp = self.vfs('fncache', mode='ab', atomictemp=True)

+ fp = self.vfs('fncache', mode='ab')

Ah, no. addbackup() creates hardlink, which means fncache can't be updated in
place. Also, the reader wouldn't handle partially-written fncache well.

do we need to call addbackup() if we are just appending?

pulkit added a comment.Nov 28 2018, 12:06 PM
Pasting Yuja's reply to that from mailing list:
In D5308#78989, @pulkit wrote:
In D5308#78985, @yuja wrote:
  • a/mercurial/store.py

+++ b/mercurial/store.py
@@ -486,7 +486,7 @@

if self.addls:
   # if we have just new entries, let's append them to the fncache
    tr.addbackup('fncache')
  • fp = self.vfs('fncache', mode='ab', atomictemp=True)

+ fp = self.vfs('fncache', mode='ab')

Ah, no. addbackup() creates hardlink, which means fncache can't be updated in
place. Also, the reader wouldn't handle partially-written fncache well.

do we need to call addbackup() if we are just appending?

We need a mechanism to restore the original content on rollback. IIUC,
it's tr.add() for append-only files.
As I said, we'll also need to somehow hide partial data from another
reader processes. There's no read lock.

Read locks! was there a previous attempt on implementing them? I will try to take a shot at implementing them. I am unable to find any good detail except mentions in wiki or mailing list archive.

yuja added a comment.Nov 29 2018, 6:59 AM
> As I said, we'll also need to somehow hide partial data from another
>  reader processes. There's no read lock.
Read locks! was there a previous attempt on implementing them? I will try to take a shot at implementing them. I am unable to find any good detail except mentions in wiki or mailing list archive.

Another option is to ignore trailing data after \n. That's how the revlog
parser gets around partially-written entry.

In either way, writing fncache non-atomically wouldn't be strictly compatible
with older hg clients. I think that's okayish, but I should note that.

baymax requested changes to this revision.Jan 24 2020, 12:32 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:32 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/store.py (2 lines)

Diff 12610

mercurial/store.py

fp = self.vfs('fncache', mode='wb', atomictemp=True) fp = self.vfs('fncache', mode='wb', atomictemp=True)
if self.entries: if self.entries:
fp.write(encodedir('\n'.join(self.entries) + '\n')) fp.write(encodedir('\n'.join(self.entries) + '\n'))
fp.close() fp.close()
self._dirty = False self._dirty = False
if self.addls: if self.addls:
# if we have just new entries, let's append them to the fncache # if we have just new entries, let's append them to the fncache
tr.addbackup('fncache') tr.addbackup('fncache')
fp = self.vfs('fncache', mode='ab', atomictemp=True) fp = self.vfs('fncache', mode='ab')
if self.addls: if self.addls:
fp.write(encodedir('\n'.join(self.addls) + '\n')) fp.write(encodedir('\n'.join(self.addls) + '\n'))
fp.close() fp.close()
self.entries = None self.entries = None
self.addls = set() self.addls = set()
def add(self, fn): def add(self, fn):
if self.entries is None: if self.entries is None: