diff --git a/mercurial/store.py b/mercurial/store.py

• a/mercurial/store.py

+++ b/mercurial/store.py
@@ -461,13 +461,13 @@

• 1. skip nonexistent file self.entries = set() return
• self.entries = set(decodedir(fp.read()).splitlines())
• if '' in self.entries:
• fp.seek(0)
• for n, line in enumerate(util.iterfile(fp)):
• if not line.rstrip('\n'):
• t = _('invalid entry in fncache, line %d') % (n + 1)
• raise error.Abort(t)

+ self.entries = set()
+ for n, line in enumerate(util.iterfile(fp)):
+ entry = line.rstrip('\n')
+ if not entry:
+ t = _('invalid entry in fncache, line %d') % (n + 1)
+ raise error.Abort(t)
+ self.entries.add(decodedir(entry))

This goes the opposite direction to 9fca5b056c0a. I guess the current code
would be faster if the fncache is around ~10MB. Can you benchmark it?

In D5296#78834, @yuja wrote:

diff --git a/mercurial/store.py b/mercurial/store.py

• a/mercurial/store.py

+++ b/mercurial/store.py
@@ -461,13 +461,13 @@

• 1. skip nonexistent file self.entries = set() return
• self.entries = set(decodedir(fp.read()).splitlines())
• if '' in self.entries:
• fp.seek(0)
• for n, line in enumerate(util.iterfile(fp)):
• if not line.rstrip('\n'):
• t = _('invalid entry in fncache, line %d') % (n + 1)
• raise error.Abort(t)

+ self.entries = set()
+ for n, line in enumerate(util.iterfile(fp)):
+ entry = line.rstrip('\n')
+ if not entry:
+ t = _('invalid entry in fncache, line %d') % (n + 1)
+ raise error.Abort(t)
+ self.entries.add(decodedir(entry))

This goes the opposite direction to 9fca5b056c0a. I guess the current code
would be faster if the fncache is around ~10MB. Can you benchmark it?

Yeah the current code is much faster if fncache is not large.

Seeing the performance benefit it brings on our repo, I want to try other ways we can do this. Do we like having a conditional which checks the size of fncache and choose one of the approaches depending on how large it is?

If size(fncache) < 50M, use the current approach
else, use the approach described in this patch

@yuja what do you think?

Ping!

I suspect 9fca5b056c0a made things faster because the code before was using 1 I/O operation for every entry. I would also not be surprised if CPython from that era did something very inefficient with regards to line reading.

The current code is pretty bad because it buffers the entire file content in memory! I agree we should change it.

I like this patch as written. If profiling shows it to be slow, I think there is room to optimize util.iterfile() or even to teach the vfs layer how to efficiently open files for line-based I/O. This is something I could help optimize if needed.

While I'm here, the fncache file being a newline delimited list of full file paths is kinda ridiculous. We could do much better by using compression and/or a more complicated data structure. It is kinda silly that we have to load this decoded data structure into memory. So if your file on disk is ~100MB, you are going to have a Python set that also consumes ~100MB. That's really not great.

(resend without the "On ... wrote:" line)

Seeing the performance benefit it brings on our repo, I want to try other ways we can do this. Do we like having a conditional which checks the size of fncache and choose one of the approaches depending on how large it is?
If size(fncache) < 50M, use the current approach
else, use the approach described in this patch

Suppose the current code is fast because it avoids running Python in loop,
I think it can be extended to not use too much memory.

chunk = b''
while True:
    chunk += fp.read(chunk_size)  # maybe ~10MB?
    p = chunk.rindex(b'\n')  # need to handle error
    decodedir(chunk[:p + 1])...
    chunk = chunk[p + 1:]

Just an idea. Not profiled.

In D5296#87892, @yuja wrote:

(resend without the "On ... wrote:" line)

Seeing the performance benefit it brings on our repo, I want to try other ways we can do this. Do we like having a conditional which checks the size of fncache and choose one of the approaches depending on how large it is?
If size(fncache) < 50M, use the current approach
else, use the approach described in this patch

Suppose the current code is fast because it avoids running Python in loop,
I think it can be extended to not use too much memory.

chunk = b''
while True:
    chunk += fp.read(chunk_size)  # maybe ~10MB?
    p = chunk.rindex(b'\n')  # need to handle error
    decodedir(chunk[:p + 1])...
    chunk = chunk[p + 1:]

Just an idea. Not profiled.

I implemented this idea. It saves ~1 sec on perffncacheload for us when chunksize is taken 1 MB.

In D5296#87890, @indygreg wrote:

I suspect 9fca5b056c0a made things faster because the code before was using 1 I/O operation for every entry. I would also not be surprised if CPython from that era did something very inefficient with regards to line reading.
The current code is pretty bad because it buffers the entire file content in memory! I agree we should change it.
I like this patch as written. If profiling shows it to be slow, I think there is room to optimize util.iterfile() or even to teach the vfs layer how to efficiently open files for line-based I/O. This is something I could help optimize if needed.
While I'm here, the fncache file being a newline delimited list of full file paths is kinda ridiculous. We could do much better by using compression and/or a more complicated data structure. It is kinda silly that we have to load this decoded data structure into memory. So if your file on disk is ~100MB, you are going to have a Python set that also consumes ~100MB. That's really not great.

I agree. We should either use compression or use a much complicated data structure here. Right now there is no way to check whether an entry exists or not without parsing the whole fncache. It does not affect much on small repositories but on large repositories such as mozilla, the difference of parsing the whole fncache is noticable. We have perffncacheload taking ~4 seconds :/

• self.entries = set(decodedir(fp.read()).splitlines())

+
+ self.entries = []
+ totalsize = self.vfs.stat('fncache').st_size

I don't think totalsize has to be stat()ed. We can just loop over until
fp.read() reaches to EOF. It's unreliable to assume that the stat('fncache')
see the identical file to the fp open()ed before.

+ chunksize = (10 ** 6) # 1 MB
+ chunk = b''
+ chunksize = min(totalsize, chunksize)

Do we have any test covering totalsize > chunksize case?

+ totalsize -= chunksize
+ while chunksize:
+ chunk += fp.read(chunksize)
+ try:
+ p = chunk.rindex(b'\n')
+ self.entries.extend(decodedir(chunk[:p + 1]).splitlines())

Any reason to not build a set directly?

+ chunk = chunk[p + 1:]
+ except ValueError:
+ # substring '\n' not found, maybe the entry is bigger than the
+ # chunksize, so let's keep iterating
+ pass
+ chunksize = min(totalsize, chunksize)
+ totalsize -= chunksize
+
+ self.entries = set(self.entries)

In D5296#88016, @yuja wrote:

• self.entries = set(decodedir(fp.read()).splitlines())

+
+ self.entries = []
+ totalsize = self.vfs.stat('fncache').st_size

I don't think totalsize has to be stat()ed. We can just loop over until
fp.read() reaches to EOF. It's unreliable to assume that the stat('fncache')
see the identical file to the fp open()ed before.

+ chunksize = (10 ** 6) # 1 MB
+ chunk = b''
+ chunksize = min(totalsize, chunksize)

Do we have any test covering totalsize > chunksize case?

Right now this patch does not have any tests. How should I add them?

1. add a debug config option and pass that to fncachestore and then to fncache
2. have a function which returns the chunk_size, write an extenion in tests which wrap that function and enable that extension in tests

I think 2) is better option here. I am unable to think of any solution expect this two. I think you will have better ideas here. What do you think?

+ for c in iter(functools.partial(fp.read, chunksize), b''):
+ chunk += c
+ try:
+ p = chunk.rindex(b'\n')
+ self.entries |= set(decodedir(chunk[:p + 1]).splitlines())

Nit: you can entries.update(any_iterable) which would be slightly faster
when bucket size changes.

Right now this patch does not have any tests. How should I add them?
1. add a debug config option and pass that to fncachestore and then to fncache
2. have a function which returns the chunk_size, write an extenion in tests which wrap that function and enable that extension in tests

Something like 2. chunksize can be a module constant, which can later be
updated by an extension. That's probably the easiest option.

Queued, thanks.

+ self.entries = set()
+ chunk = b''
+ for c in iter(functools.partial(fp.read, fncache_chunksize), b''):
+ chunk += c
+ try:
+ p = chunk.rindex(b'\n')
+ self.entries.update(decodedir(chunk[:p + 1]).splitlines())
+ chunk = chunk[p + 1:]
+ except ValueError:
+ # substring '\n' not found, maybe the entry is bigger than the
+ # chunksize, so let's keep iterating
+ pass

We might want to check if the chunk is fully consumed. If the file doesn't
end with '\n', which I think is invalid though, the last line would be silently
ignored.