config_file: implement stat cache to avoid repeated rehashing by pks-t · Pull Request #5132 · libgit2/libgit2

pks-t · 2019-06-21T10:21:00Z

To decide whether a config file has changed, we always hash its
complete contents. This is unnecessarily expensive, as
well-behaved filesystems will always update stat information for
files which have changed. So before computing the hash, we should
first check whether the stat info has actually changed for either
the configuration file or any of its includes. This avoids having
to re-read the configuration file and its includes every time
when we check whether it's been modified.

Tracing the for-each-ref example previous to this commit, one can
see that we repeatedly re-open both the repo configuration as
well as the global configuration:

    $ strace lg2 for-each-ref |& grep config
    access("/home/pks/.gitconfig", F_OK)    = -1 ENOENT (No such file or directory)
    access("/home/pks/.config/git/config", F_OK) = 0
    access("/etc/gitconfig", F_OK)          = -1 ENOENT (No such file or directory)
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    access("/tmp/repo/.git/config", F_OK)   = 0
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/home/pks/.gitconfig", 0x7ffd15c05290) = -1 ENOENT (No such file or directory)
    access("/home/pks/.gitconfig", F_OK)    = -1 ENOENT (No such file or directory)
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    access("/home/pks/.config/git/config", F_OK) = 0
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/home/pks/.gitconfig", 0x7ffd15c051f0) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/home/pks/.gitconfig", 0x7ffd15c05090) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/home/pks/.gitconfig", 0x7ffd15c05090) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/home/pks/.gitconfig", 0x7ffd15c05090) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3

With the change, we only do stats for those files and open them a
single time, only:

    $ strace lg2 for-each-ref |& grep config
    access("/home/pks/.gitconfig", F_OK)    = -1 ENOENT (No such file or directory)
    access("/home/pks/.config/git/config", F_OK) = 0
    access("/etc/gitconfig", F_OK)          = -1 ENOENT (No such file or directory)
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    access("/tmp/repo/.git/config", F_OK)   = 0
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/home/pks/.gitconfig", 0x7ffe70540d20) = -1 ENOENT (No such file or directory)
    access("/home/pks/.gitconfig", F_OK)    = -1 ENOENT (No such file or directory)
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    access("/home/pks/.config/git/config", F_OK) = 0
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    stat("/home/pks/.gitconfig", 0x7ffe70540ca0) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.gitconfig", 0x7ffe70540c80) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    stat("/home/pks/.gitconfig", 0x7ffe70540b40) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.gitconfig", 0x7ffe70540b20) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    stat("/home/pks/.gitconfig", 0x7ffe70540b40) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.gitconfig", 0x7ffe70540b20) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0
    stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0
    stat("/home/pks/.gitconfig", 0x7ffe70540b40) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.gitconfig", 0x7ffe70540b20) = -1 ENOENT (No such file or directory)
    stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0

The following benchmark has been performed with and without the
stat cache in a best-of-ten run:


int lg2_repro(git_repository *repo, int argc, char **argv)
{
        git_config *cfg;
        int32_t dummy;
        int i;

        UNUSED(argc);
        UNUSED(argv);

        check_lg2(git_repository_config(&cfg, repo),
                        "Could not obtain config", NULL);

        for (i = 1; i < 100000; ++i)
                git_config_get_int32(&dummy, cfg, "foo.bar");

        git_config_free(cfg);
        return 0;
}

Without stat cache:

    $ time lg2 repro
    real    0m1.528s
    user    0m0.568s
    sys     0m0.944s

With stat cache:

    $ time lg2 repro
    real    0m0.526s
    user    0m0.268s
    sys     0m0.258s

This benchmark shows a nearly three-fold performance improvement.

pks-t · 2019-06-21T10:22:39Z

Fixes #5126? At least parts of it, I guess.

pks-t · 2019-06-21T11:56:18Z

So this in fact breaks Windows-based systems when reading and writing files at nearly the same time due to Windows usually not using sub-second file stats and thus we're racy. To be honest, I don't think this matters and I'd still like to get this merged. If you have two processes reading and writing files at nearly the same time, then I find it totally reasonable to say "Yup, this is racy without any external synchronization mechanism."

mehrdadn · 2019-06-21T11:58:41Z

@pks-t: It seems to avoid the reading of the files (which is great!), but their opening or most other calls---those are still extremely slow. The change was only a handful of FS calls.

An easy (although conservative) way to avoid this in many situations on Windows is to do the following: the very first time the .git directory is accessed, you call handle = FindFirstChangeNotification(".git", TRUE, flags) with all possible filter flags set. What you then do is, every time you would have re-opened a file or tried to stat it, you now instead just call WaitForSingleObject(handle, 0) to see if the wait is satisfied. If the wait is satisfied (i.e. doesn't time out), then this means something has changed, and you can do all the stat/reloading checks then, then call FindNextChangeNotification to wait for new changes. But if it isn't, then you avoid hitting the file system entirely and assume the cache is good.

For .gitattributes you could do the same with the worktree directory rather than the git directory. If the git directory is inside the worktree as normal then you could even just combine them into one if you wanted, although that's probably overkill.

There's also a harder way to accomplish something similar, which is to keep the config files actually open, then use them directly (e.g. fstat) on subsequent calls. That's the "proper" way to access files in general. I expect (though I'm not sure) that it wouldn't be as fast as the above approach since it would still hit the file system drivers, but it would still be much faster than what's currently being done, since the file doesn't need to be reopened every time. However, it can lock the config files and prevent them from being replaced or modified. That may be either more or less user-friendly depending on your point of view. Personally I think the FindFirstChangeNotification approach is the way to go, but they're both improvements and in fact not even mutually exclusive, so both of them could be done independently if desired (and they're both performance improvements over the current implementation).

The above can be in a separate PR; doesn't have to be this one. Just wanted to explain how to do it, since at least the change notification method is quite easy. (You don't even need to implement it on non-Windows systems if you don't want, since just stat'ing like usual on those is probably already probably fast enough.)

pks-t · 2019-06-21T15:17:57Z

This is the general and non-Windows-specific approach. Yours just wouldn't work in a cross-platform way, at least as far as I know. If there is a cross-platform way then I'd certainly be happy to hear about it

However, it can lock the config files and prevent them from being replaced or modified.

Yeah, that'd be a huge problem. Many IDEs or editors just keep repos open all the time while the editor is alive, and we just cannot lock any files like that.

mehrdadn · 2019-06-21T15:21:04Z

So what if it's a platform specific approach though? Just #ifdef _WIN32.

pks-t · 2019-06-21T15:25:18Z

So what if it's a platform specific approach though? Just #ifdef _WIN32.

Yeah, maybe in another increment. I'm not a Windows guy and this here already improves runtime performance by a lot on all platforms.

mehrdadn · 2019-06-21T15:27:02Z

Oh yeah, like I said, it doesn't need to be on this PR or anything. I just mentioned it as a TODO so we don't forget. I might be able to help with the Windows parts too later.

pks-t · 2019-06-21T16:01:04Z

It seems to avoid the reading of the files (which is great!), but their opening or most other calls---those are still extremely slow. The change was only a handful of FS calls.

By the way: this isn't quite true. Yes, it only removes a handful of calls, but in fact it removes the most hurting ones. Reads are always a lot less efficient compared to stats, but most importantly we now completly avoid re-hashing the file contents to see whether they're still the same. Previously, we would've always hashed the contents.

pks-t · 2019-06-21T16:02:20Z

At a later point we might decide to roll this into our filebuffers. It seems like a good match to have stuff like that as part of them, as there's multiple locations where we use this sort of caching. Furthermore, that'd allow us to more easily implement platform-specific heuristics similar to the one proposed by @mehrdadn

mehrdadn · 2019-06-21T21:35:37Z

Reads are always a lot less efficient compared to stats,

@pks-t: No, it's not wrong. I've been trying to tell this to all of you on both threads, but it seems you still don't realize the sheer magnitude of the numbers. So let me put it in terms of actual numbers.

Here's what it looks like on a real benchmark:

Windows:

30.15 us/call access()
34.55 us/call stat()
30.27 us/call open()/close()
35.46 us/call open()/fstat()/close()
36.38 us/call open()/read(16k)/close()
Linux:

0.81 us/call access()
0.52 us/call stat()
1.28 us/call open()/close()
1.72 us/call open()/fstat()/close()
2.47 us/call open()/read(16k)/close()

Here's what you should be observing:

On Windows, stat is slower than just opening/closing the file.
On Windows, stat is over 8x slower than reading 16k of data.
On Linux, stat is less than 5x faster than reading 16k of data.
On Linux, stat are merely 2.5x faster than open/close.
On Linux, stat is 66x FASTER than on Windows.
On Windows, the overhead of accessing a file by path is 25 TIMES more than on Linux, and dominates the I/O time.
On both platforms, read() has approximately the same performance.

So you can see stat is pretty comparable with read (Linux), if not significantly slower (Windows), until you start getting into the 100+ KB range. And I'm sure you realize that the average file on git is smaller than that, so this is the uncommon case.

Now maybe try benchmarking this yourself your Mac and letting me know what you see:

#include <stddef.h>
#include <stdlib.h>
#include <stdio.h>
#ifdef _WIN32
#include <io.h>
#else
#include <unistd.h>
#define O_BINARY 0
#endif
#include <errno.h>
#include <fcntl.h>
#include <time.h>
#include <sys/stat.h>

int fileperftest()
{
	char const *const name = "testfile123";
	size_t const count =
#ifdef _WIN32
		1 << 14
#else
		1 << 20
#endif
		;
	for (int op = -2; op <= 2; ++op)
	{
		for (int trial = 0; trial < 1; ++trial)
		{
			unsigned int bufsize = 1 << 14;
			void *const buf = malloc(bufsize);
			{
				int const fd = open(name, O_BINARY | O_CREAT | O_WRONLY | O_TRUNC, S_IREAD | S_IWRITE);
				if (fd != -1)
				{
					if (write(fd, buf, bufsize) == -1) { return errno; }
					close(fd);
				}
				else { return errno; }
			}
			clock_t const tbegin = clock();
			for (size_t i = 0; i != count; ++i)
			{
				if (op == -2)
				{
					if (access(name, 0) == -1)
					{ return errno; }
				}
				else if (op == -1)
				{
					struct stat stat_;
					if (stat(name, &stat_) == -1)
					{ return errno; }
				}
				else
				{
					int const fd = open(name, O_BINARY | O_RDONLY);
					if (fd != -1)
					{
						if (op == 1) { struct stat stat_; fstat(fd, &stat_); }
						else if (op == 2) { read(fd, buf, bufsize); }
						close(fd);
					}
					else { return errno; }
				}
			}
			unlink(name);
			free(buf);
			fprintf(stdout, "%.2f us/call %s\n",
				(clock() - tbegin) * 1000000.0 / (CLOCKS_PER_SEC * count),
				op == -2 ? "access()" : op == -1 ? "stat()" : op == 0 ? "open()/close()" : op == 1 ? "open()/fstat()/close()" : "open()/read()/close()");
			fflush(stdout);
		}
	}
	return 0;
}

int main() { return fileperftest(); }

ethomson · 2019-06-22T17:23:39Z

Now maybe try benchmarking this yourself your Mac and letting me know what you see

I don’t understand why you make any assumption about what technologies any of us use.

mehrdadn · 2019-06-22T18:13:52Z

Now maybe try benchmarking this yourself your Mac and letting me know what you see

I don’t understand why you make any assumption about what technologies any of us use.

@ethomson well because he said "I'm not a Windows guy". As to why Mac and not Linux, though, I thought he was the one who said he's the MacOS developer but turns out it was tiennou who said that on the other thread, so I misremembered that bit.

But I'm not "making assumptions", I'm just going by what you yourselves have told me!

ethomson · 2019-06-23T13:39:41Z

So this in fact breaks Windows-based systems when reading and writing files at nearly the same time due to Windows usually not using sub-second file stats and thus we're racy.

Indeed it does seem to. We could add a 100 ns sleep on win32 to that config test, which should ensure that we actually update the filetime.

mehrdadn · 2019-06-23T18:15:58Z

So this in fact breaks Windows-based systems when reading and writing files at nearly the same time due to Windows usually not using sub-second file stats and thus we're racy.

Indeed it does seem to. We could add a 100 ns sleep on win32 to that config test, which should ensure that we actually update the filetime.

Sleeping for 100ns on Windows is not as easy as it sounds. You'll have to spin loop and remeasure.

Also, I think he meant stat's precision is 1 second, not 100 nanoseconds. For the latter you'll want to use something like FindFirstFile or GetFileTime.

ethomson · 2019-06-23T18:30:58Z

Also, I think he meant stat's precision is 1 second, not 100 nanoseconds. For the latter you'll want to use something like FindFirstFile or GetFileTime.

Our stat emulation uses a FILETIME.

mehrdadn · 2019-06-23T18:32:04Z

Oh I see, cool.

pks-t · 2019-06-24T05:26:09Z

On Fri, Jun 21, 2019 at 02:35:47PM -0700, mehrdadn wrote: > Reads are always a lot less efficient compared to stats, @pks-t: No, it's *not* wrong. I've been trying to tell this to all of you on both threads, but it seems you still don't realize the sheer magnitude of the numbers. So let me put it in terms of actual numbers for.

"Not quite true" and "wrong" are different things. Furthermore I claimed that the most important part is that we stop re-hashing contents.

pks-t · 2019-06-24T06:03:01Z

@ethomson: rebased. I've introduced a 1ms sleep, which should probably fix the test

pks-t · 2019-06-24T06:53:39Z

Changed to 10ms due to it still being flaky

ethomson · 2019-06-24T09:05:01Z

"Not quite true" and "wrong" are different things. Furthermore I
claimed that the most important part is that we stop re-hashing
contents.

I'd like to hit pause here for a second. ⏸

Things over here in the libgit2 community have seemed a bit contentious recently, and I'd like to nip this in the bud and make sure that we all get on the same page.

Obviously, it's very difficult to convey things like tone online. So things that are meant as jokes may not come across that way, and readers may miss the intent and instead hear attacks. Things that are meant as emphasis may be interpreted as rudeness.

So a quick reminder about our code of conduct, which reminds us that this aims to be a positive environment, and that we should all be:

Using welcoming and inclusive language
Being respectful of differing viewpoints and experiences
Gracefully accepting constructive criticism
Focusing on what is best for the community
Showing empathy towards other community members

And a reminder that our goals are to build a Git repository management library that is first correct and second performant, and that we need to work together to do that. This is an all-volunteer project, and we're all here because we want to be. We don't need to argue about degrees of incorrectness, and we don't need comments that can be perceived as rude. We need collaboration and inclusiveness.

Thanks in advance!

pks-t · 2019-06-24T14:09:04Z

@ethomson: are you good with these changes and opening up the potential for races?

ethomson · 2019-06-24T14:20:11Z

I think so. For the filesystem-based config system, it's hard for me to imagine that

But we could add some tuning to the FS-based config system to disable stat in favor of a more heavy handed approach (to minimize that NTFS attribute update lag, or if they're trying to use something worse even than NTFS) or to disable reloading entirely.

But I think that this is good for the defaults. 👍

pks-t · 2019-06-24T15:59:38Z

But we could add some tuning to the FS-based config system to disable stat in favor of a more heavy handed approach (to minimize that NTFS attribute update lag, or if they're trying to use something worse even than NTFS) or to disable reloading entirely.

Well, the more heavy handed approach is the one we had before, right? :) We could use a global option to switch behaviour here, of course. Did you mean to imply that?

mehrdadn · 2019-06-24T20:56:36Z

But we could add some tuning to the FS-based config system to disable stat in favor of a more heavy handed approach (to minimize that NTFS attribute update lag, or if they're trying to use something worse even than NTFS) or to disable reloading entirely.

I got confused and didn't really follow what's happening here, but just a note: as far as I know (although I'm not 100% sure about this) there shouldn't be a "lag" per se in NTFS file time updating. Rather, the situation is like this:

When writing to a file, the last write time is not fully updated until all handles that are used for writing are closed.

So to update the file time I'd expect the only thing to do is to just close all file descriptors used for writing. I think you shouldn't need to change how you actually read the file time (whether it's via stat or FindFirstFile or something else) and I think there shouldn't be a lag in between. If you're seeing a lag that isn't accounted for in this way, please do send me a repro so I can see if there's a better way to resolve it...

And here's a demo that illustrates this:

#!/usr/bin/env python3

import os

def stat_via_dir(filename):
	basename = os.path.basename(filename)
	with os.scandir(os.path.dirname(filename) or '.') as it:
		for entry in it:
			if entry.name == basename:
				return entry.stat()

def main(program, filename='testfile'):
	print(os.stat(filename).st_mtime)
	with open(filename, 'r+b') as f:
		print(stat_via_dir(filename).st_mtime)
		f.write(b'\n')
		f.flush()
		print(os.stat(filename).st_mtime)
		print(stat_via_dir(filename).st_mtime)
	print(os.stat(filename).st_mtime)
	print(stat_via_dir(filename).st_mtime)

if __name__ == '__main__':
	import sys
	raise SystemExit(main(*sys.argv))

pks-t · 2019-06-27T07:46:04Z

Updated. I've optimized how we handle refreshes after calling config_write so that we do not have to re-read the on-disk file but instead handle this from the in-memory buffer which we had already available anyway. This avoids any timing issues due to my stat cache when writing config files and should thus fix any spurious errors introduced by it. I've thus lowered the sleep to 1ms again, which should now be sufficient

mehrdadn · 2019-06-27T07:48:13Z

Awesome :) is the 1ms sleep due to the 100ns (and the fact that it's hard to go below 1ms)? Or do you need it to be actually 1ms long? Because if you only need the 100ns resolution I could write you a faster sleep...

pks-t · 2019-06-27T07:56:16Z

Awesome :) is the 1ms sleep due to the 100ns (and the fact that it's hard to go below 1ms)? Or do you need it to be actually 1ms long? Because if you only need the 100ns resolution I could write you a faster sleep...

The first reason was that implementing cl_msleep is trivial on both Win32 and Unix by mapping to Sleep(x) and usleep(1000*x), respectively. So like this we didn't need much code, especially considering that it's of limited use for our tests, only. And delaying tests by 10 ms in total is probably not really going to be noticeable. Especially not on Windows, which is slow as hell anyway when it comes to test suite execution :(

The second reason was that we also need to consider systems built with "-DUSE_NSEC=NO", where we might not even have that level of granularity for fstat. In that case, sleeping for less than a millisecond would possibly cause the stat cache to trip again.

So thanks for your offer, but I don't really think it's necessary for now :)

tiennou

Nothing really stood out, so LGTM (though there's a build failure on clang). Thanks for tackling this @pks-t !

tiennou · 2019-07-09T12:38:07Z

+
+static int config_refresh(git_config_backend *cfg)
+{
+	diskfile_backend *b = (diskfile_backend *)cfg;


Wouldn't that warrant one of those GIT_CONTAINER_OF things, just in case ?

Definitely. Embarassing that I myself didn't think about it :P

Depending on the platform and on build options, we may or may not build libgit2 with support for nanoseconds when using `stat` calls. It's currently unclear though whether sub-second stat information is used at all. Add feature info for this to tell at configure time whether it's being used or not.

Implement a new example that resembles git-config(1). Right now, this example can both read and set configuration keys, only.

To decide whether a config file has changed, we always hash its complete contents. This is unnecessarily expensive, as well-behaved filesystems will always update stat information for files which have changed. So before computing the hash, we should first check whether the stat info has actually changed for either the configuration file or any of its includes. This avoids having to re-read the configuration file and its includes every time when we check whether it's been modified. Tracing the for-each-ref example previous to this commit, one can see that we repeatedly re-open both the repo configuration as well as the global configuration: $ strace lg2 for-each-ref |& grep config access("/home/pks/.gitconfig", F_OK) = -1 ENOENT (No such file or directory) access("/home/pks/.config/git/config", F_OK) = 0 access("/etc/gitconfig", F_OK) = -1 ENOENT (No such file or directory) stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 access("/tmp/repo/.git/config", F_OK) = 0 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/home/pks/.gitconfig", 0x7ffd15c05290) = -1 ENOENT (No such file or directory) access("/home/pks/.gitconfig", F_OK) = -1 ENOENT (No such file or directory) stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 access("/home/pks/.config/git/config", F_OK) = 0 stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/home/pks/.gitconfig", 0x7ffd15c051f0) = -1 ENOENT (No such file or directory) stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/home/pks/.gitconfig", 0x7ffd15c05090) = -1 ENOENT (No such file or directory) stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/home/pks/.gitconfig", 0x7ffd15c05090) = -1 ENOENT (No such file or directory) stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/home/pks/.gitconfig", 0x7ffd15c05090) = -1 ENOENT (No such file or directory) stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3 With the change, we only do stats for those files and open them a single time, only: $ strace lg2 for-each-ref |& grep config access("/home/pks/.gitconfig", F_OK) = -1 ENOENT (No such file or directory) access("/home/pks/.config/git/config", F_OK) = 0 access("/etc/gitconfig", F_OK) = -1 ENOENT (No such file or directory) stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 access("/tmp/repo/.git/config", F_OK) = 0 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/home/pks/.gitconfig", 0x7ffe70540d20) = -1 ENOENT (No such file or directory) access("/home/pks/.gitconfig", F_OK) = -1 ENOENT (No such file or directory) stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 access("/home/pks/.config/git/config", F_OK) = 0 stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 stat("/home/pks/.gitconfig", 0x7ffe70540ca0) = -1 ENOENT (No such file or directory) stat("/home/pks/.gitconfig", 0x7ffe70540c80) = -1 ENOENT (No such file or directory) stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 stat("/home/pks/.gitconfig", 0x7ffe70540b40) = -1 ENOENT (No such file or directory) stat("/home/pks/.gitconfig", 0x7ffe70540b20) = -1 ENOENT (No such file or directory) stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 stat("/home/pks/.gitconfig", 0x7ffe70540b40) = -1 ENOENT (No such file or directory) stat("/home/pks/.gitconfig", 0x7ffe70540b20) = -1 ENOENT (No such file or directory) stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 stat("/tmp/repo/.git/config", {st_mode=S_IFREG|0644, st_size=92, ...}) = 0 stat("/home/pks/.gitconfig", 0x7ffe70540b40) = -1 ENOENT (No such file or directory) stat("/home/pks/.gitconfig", 0x7ffe70540b20) = -1 ENOENT (No such file or directory) stat("/home/pks/.config/git/config", {st_mode=S_IFREG|0644, st_size=1154, ...}) = 0 The following benchmark has been performed with and without the stat cache in a best-of-ten run: ``` int lg2_repro(git_repository *repo, int argc, char **argv) { git_config *cfg; int32_t dummy; int i; UNUSED(argc); UNUSED(argv); check_lg2(git_repository_config(&cfg, repo), "Could not obtain config", NULL); for (i = 1; i < 100000; ++i) git_config_get_int32(&dummy, cfg, "foo.bar"); git_config_free(cfg); return 0; } ``` Without stat cache: $ time lg2 repro real 0m1.528s user 0m0.568s sys 0m0.944s With stat cache: $ time lg2 repro real 0m0.526s user 0m0.268s sys 0m0.258s This benchmark shows a nearly three-fold performance improvement. This change requires that we check our configuration stress tests as we're now in fact becoming more racy. If somebody is writing a configuration file at nearly the same time (there is a window of 100ns on Windows-based systems), then it might be that we realize that this file has actually changed and thus may not re-read it. This will only happen if either an external process is rewriting the configuration file or if the same process has multiple `git_config` structures pointing to the same time, where one of both is being used to write and the other one is used to read values.

We are quite lazy in how we refresh our config file backend when updating any of its keys: instead of just updating our in-memory representation of the keys, we just discard the old set of keys and then re-read the config file contents from disk. This refresh currently happens separately at every callsite of `config_write`, but it is clear that we _always_ want to refresh if we have written the config file to disk. If we didn't, then we'd run around with an outdated config file backend that does not represent what we have on disk. By moving the refresh into `config_write`, we are also able to optimize the case where the config file is currently locked. Before, we would've tried to re-read the file even if we have only updated its cached contents without touching the on-disk file. Thus we'd have unnecessarily stat'd the file, even though we know that it shouldn't have been modified in the meantime due to its lock.

The `config_read` function currently performs both reading the on-disk config file as well as parsing the retrieved buffer contents. To optimize how we refresh our config entries from an in-memory buffer, we need to be able to directly parse buffers, though, without involving any on-disk files at all. Extract a new function `config_read_buffer` that sets up the parsing logic and then parses config entries from a buffer, only. Have `config_read` use it to avoid duplicated logic.

Updating a config file backend's config entries is a bit more involved, as it requires clearing of the old config entries as well as handling locking correctly. As we will need this functionality in a future patch to refresh config entries from a buffer, let's extract this into its own function `config_set_entries`.

When we rewrite the configuration file due to any of its values being modified, we call `config_refresh` to update the in-memory representation of our config file backend. This is needlessly wasteful though, as `config_refresh` will always open the on-disk representation to reads the file contents while we already know the complete file contents at this point in time as we have just written it to disk. Implement a new function `config_refresh_from_buffer` that will refresh the backend's config entries from a buffer instead of from the config file itself. Note that this will thus _not_ update the backend's timestamp, which will cause us to re-read the buffer when performing a read operation on it. But this is still an improvement as we now lazily re-read the contents, and most importantly we will avoid constantly re-reading the contents if we perform multiple write operations. The following strace demonstrates this if we're re-writing a key multiple times. It uses our config example with `config_set` changed to update the file 10 times with different keys: $ strace lg2 config x.x z |& grep '^open.*config' open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 And now with the optimization of `config_refresh_from_buffer`: $ strace lg2 config x.x z |& grep '^open.*config' open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 3 open("/home/pks/.config/git/config", O_RDONLY|O_CLOEXEC) = 3 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 open("/tmp/repo/.git/config.lock", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 open("/tmp/repo/.git/config", O_RDONLY|O_CLOEXEC) = 4 As can be seen, this is quite a lot of `open` calls less.

pks-t · 2019-07-11T06:38:56Z

Thanks for your review, @tiennou! All comments should be addressed now

tiennou · 2019-07-11T08:39:23Z

+		if ((result = git_filebuf_commit(&file)) < 0)
+			goto done;
+
+		if ((result = config_refresh_from_buffer(&cfg->header.parent, buf.ptr, buf.size)) < 0)


Build is still red, because Windows fails and/or leaks. I've looked around for the leak and pinpointed one possible occurrence here, but since there's also timing issues at hand, maybe not ?

buf was "attached" in the other code path, but not here?

Nah, that was just a quick test of mine :P Reverted it again to the previous state, which passes

pks-t · 2019-07-11T08:49:00Z

Thanks for your reviews, @tiennou!

pks-t force-pushed the pks/config-stat-cache branch 2 times, most recently from cf43e99 to 3e55114 Compare June 21, 2019 10:31

pks-t mentioned this pull request Jun 21, 2019

Config parser separation #5134

Merged

pks-t force-pushed the pks/config-stat-cache branch 2 times, most recently from 26ba376 to 3ce5844 Compare June 24, 2019 06:02

pks-t force-pushed the pks/config-stat-cache branch from 3ce5844 to 1725582 Compare June 24, 2019 06:33

pks-t force-pushed the pks/config-stat-cache branch 2 times, most recently from e99d9c4 to caeb80f Compare June 27, 2019 07:41

pks-t force-pushed the pks/config-stat-cache branch 2 times, most recently from 5b208e7 to f0a2653 Compare June 27, 2019 12:21

tiennou approved these changes Jul 9, 2019

View reviewed changes

pks-t added 8 commits July 11, 2019 08:28

examples: implement config example

8ee3d39

Implement a new example that resembles git-config(1). Right now, this example can both read and set configuration keys, only.

config: use git_config_file in favor of struct config_file

d086864

pks-t force-pushed the pks/config-stat-cache branch from f0a2653 to 2ba7020 Compare July 11, 2019 06:38

pks-t force-pushed the pks/config-stat-cache branch 2 times, most recently from 2b50aac to 2ba7020 Compare July 11, 2019 08:31

tiennou reviewed Jul 11, 2019

View reviewed changes

pks-t merged commit ba9725a into libgit2:master Jul 11, 2019

pks-t deleted the pks/config-stat-cache branch July 11, 2019 08:48

This was referenced Jul 18, 2019

git_remote_list does not detect config changes on disk #5167

Closed

config_file: refresh when creating an iterator #5181

Merged

implausible mentioned this pull request Jul 23, 2019

Bump libgit2 nodegit/nodegit#1705

Merged

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Conversation

pks-t commented Jun 21, 2019

Uh oh!

pks-t commented Jun 21, 2019

Uh oh!

pks-t commented Jun 21, 2019

Uh oh!

mehrdadn commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pks-t commented Jun 21, 2019

Uh oh!

mehrdadn commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pks-t commented Jun 21, 2019

Uh oh!

mehrdadn commented Jun 21, 2019

Uh oh!

pks-t commented Jun 21, 2019

Uh oh!

pks-t commented Jun 21, 2019

Uh oh!

mehrdadn commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethomson commented Jun 22, 2019

Uh oh!

mehrdadn commented Jun 22, 2019

Uh oh!

ethomson commented Jun 23, 2019

Uh oh!

mehrdadn commented Jun 23, 2019

Uh oh!

ethomson commented Jun 23, 2019

Uh oh!

mehrdadn commented Jun 23, 2019

Uh oh!

pks-t commented Jun 24, 2019 via email

Uh oh!

pks-t commented Jun 24, 2019

Uh oh!

pks-t commented Jun 24, 2019

Uh oh!

ethomson commented Jun 24, 2019

Uh oh!

pks-t commented Jun 24, 2019

Uh oh!

ethomson commented Jun 24, 2019

Uh oh!

pks-t commented Jun 24, 2019

Uh oh!

mehrdadn commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pks-t commented Jun 27, 2019

Uh oh!

mehrdadn commented Jun 27, 2019

Uh oh!

pks-t commented Jun 27, 2019

Uh oh!

tiennou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tiennou Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

pks-t Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pks-t commented Jul 11, 2019

Uh oh!

tiennou Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

pks-t Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

mehrdadn commented Jun 21, 2019 •

edited

Loading

mehrdadn commented Jun 21, 2019 •

edited

Loading

mehrdadn commented Jun 21, 2019 •

edited

Loading

mehrdadn commented Jun 24, 2019 •

edited

Loading