-
Notifications
You must be signed in to change notification settings - Fork 83
Handle missing and changed includes in direct mode #179
Conversation
Current coverage is 78.30% (diff: 65.45%)@@ master #179 diff @@
==========================================
Files 1 1
Lines 929 954 +25
Methods 0 0
Messages 0 0
Branches 155 159 +4
==========================================
+ Hits 730 747 +17
- Misses 166 171 +5
- Partials 33 36 +3
|
4f8b03e
to
25b5f0d
Compare
I just realized that it is perfectly possible that one given manifest contains different includes lists.
Let I do not really have an idea how to solve this properly. |
Adding the includes list to |
f64f635
to
780cece
Compare
c14cc9d
to
6ae9dc4
Compare
@webmaster128 Can you describe why we have includes list in first place? Why do we need all this manifest magic? |
Sure. When the cache sees a compile request it needs to know whether or not the resulting object is already in cache. The source filepath is not enough, because the source file content can change. Thus we hash the source file content. But even when the source file did not change, we might need to recompile because an included file (typically a header file) is changed. There are two ways to determine if source and includes are unchanged:
Mode 2. is known as DIRECT MODE and 1. is known as NODIRECT MODE. Even if nodirect is a bit more solid, it is slower than direct mode. In our setup (strong could CPU and RAM, slow NAS storage) direct mode reduces restore time of the project about 1/3. In setups where reading the include file is cheep compared to compute power (SSD or RAM disk), I expect direct mode to be even more valuable. Dealing with non-existing includes is not trivial. I took me a while to break it down to two essential tests but I don't know how to implement it. Some time ago it was solved in a way that was probably incorrect. At the moment you get crashes for every missing include file. To work around that, disable direct mode for now by setting the environment variable |
But I think I got it now: include changed must have priority over include missing. This can be archived by storing hashes with the includes list and when iterating over it, store a includeNotFound flag, which is used when no include changed. As soon as one include is changed, this has priority and the includeNotFound is discarded. I'll try to implement this within a couple of days. |
61e076c
to
f524e4f
Compare
f524e4f
to
a91839c
Compare
a91839c
to
dcfe16d
Compare
2226030
to
39765c2
Compare
I got new exception two times on different nodes: Traceback (most recent call last):
File "clcache.py", line 1568, in <module>
File "clcache.py", line 1447, in main
File "clcache.py", line 1477, in processCompileRequest
File "clcache.py", line 1547, in processDirect
File "clcache.py", line 1530, in <lambda>
File "clcache.py", line 1358, in postprocessHeaderChangedMiss
File "clcache.py", line 1295, in addObjectToCache
File "clcache.py", line 282, in setEntry
File "clcache.py", line 777, in copyOrLink
FileExistsError: [WinError 183] Cannot create a file when that file already exists: 'C:\\clcache\\objects\\4c\\4cdbbffca1bd7564d2756447347f0389\\object.tmp' -> 'C:\\clcache\\objects\\4c\\4cdbbffca1bd7564d2756447347f0389\\object'
Failed to execute script clcache Maybe such error should be skipped if content is same? |
There is indeed some time between hasEntry() and addObjectToCache(), such that this situation can happen. But I think this is an entirely independent issue. Could you please open a ticket for this? |
It's kinda hard to review this PR with so much action going on. I think I have a good idea of what problem this PR is trying to solve, and I'm very happy that there's a test for this - so I'm tempted to just merge this, if it's ready to go? |
I think you can. Tested the patch with a real project and other contributors tested it as well. |
Merged, thanks everyone involved for the work! Let's see how this goes. |
I think a regression was introduced here. Previously postprocessHeaderChangedMiss received a manifest object already initialized with the current contents of the manifest file. In this PR instead a new manifest is created from scratch, removing from the manifest references to any other object that had the same includes but different hash for the contents of the includes. |
Yes it is. In order to archive correctness, a missing optimization was introduced. Alternating include contents are overriding each other, triggering a recompile. |
The "issue" I have with this is that that is a very common case when switching branches. It could be solved quickly by reading the contents of the manifest file from disk before updating + writing the updated one. The cache would still be correct. |
I was aware of this but then moved it to the low-priority list, given that having crashlessness and correctness is more important than this (I guess) edge-case optimization. Let me add a PR adding a test for this, where we can continue the discussion. UPDATE: thanks for pointing out the use case. I did not think about switching branches. |
Please take also a look at my comment in #208 #208 (comment) |
Closes #200.
Closes #209.