-
Notifications
You must be signed in to change notification settings - Fork 473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Port the inode cache to Windows #1388
Conversation
3cb8bc3
to
4aceb04
Compare
Looks interesting, thanks for working on this! I don't have much bandwidth to look more closely at the moment, but a couple of comments after glancing over the diff very quickly:
|
I have, and at first I didn't really get what was meant but I think I get it and the issue is outdated. The current implementation of
Sure, I'll add a mention that support is experimental and off by default
Yes, I thought of that too at some point, I'll do that and document it as such |
bfa6669
to
69e05d4
Compare
69e05d4
to
a68adb8
Compare
I haven't tried it on Windows, but looks good to me. Thanks! |
I did and it's great, just works, this setting would deserve a better review or test if it contains any problems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this PR, debugged it line by line, checked all win API parameters, destroying, closing handles, copy/move operations, and didn't find even one bug or problem, @aTom3333 did a great job. It took me ~4 hours to revisit it.
I found a few tidbits like C type cast instead of reinterpret_cast<> or missing DWORD static_cast<> but that's all.
I will propose a PR for these (if I have time) but the code is correct even without them, they are only cosmetic changes.
I also didn't realize that 4.10 will be the first release that will contain it, I thought it was released earlier so enabling it by default in the first release wouldn't be a good idea.
Now I have also tested the inode cache on msys2 and it works as expected but the perf. boost is practically 0%, it's like 1s up/down, max. perf. boost I saw was 3s, but that could be expected, as msys2 FS API is much slower. Important is that it has no bugs 👌, at least I haven't discovered any. |
This PR ports the existing InodeCache to make it work under Windows.
This speeds up ccache a lot, both in the hit and miss cases, when a lot of headers files are included in several compilation units. In our case this makes the difference between ccache being worth using or not.
The logic of the InodeCache itself is unchanged. I introduced a MemoryMap helper class that does the platform specific calls to mmap/MapViewOfFile. Despite its generic name, I've implemented what was needed for the InodeCache, it only supports mapping a file (no anonymous mmap) in a shared fashion with read/write access.
Due to differences in behavior between unix and Windows regarding unlinking a file that is opened, I am not convinced that it will work correctly in every situation (for example if a process hold a bucket lock for more than 5 seconds, the cache is dropped and recreated, under Windows that won't work as the unlink will fail). That being said it seems to work well enough when nothing unexpected happens.
I made a small change to the unit test because under Windows, we can't open() a directory so instead I try to open a temp file in that directory. After that change the unit tests pass unmodified.
For the bash based test, only the symbolic link test fails. This is quite strange, when I try to run the test, the call to
ln -fs test1.c test2.c
fails, but when run manually the same command succeeds. I tried to run the test manually and it fails anyway but I haven't done anything to fix it, this looks like more an issue from the DirEntry side than the InodeCache side. Because of that I've left the bash based test disabled for Windows, maybe we want to change that to only disable the symbolic link test?