Remove temp paths from ModifiedPathsDatabase by kewillford · Pull Request #207 · microsoft/VFSForGit

kewillford · 2018-08-23T00:30:12Z

Builds and other processes will write temp files or create temp directories in the source directory which then adds the file or folder path to the ModifiedPathsDatabase. This causes performance issues because the ModifiedPathsDatabase is currently append only.

This PR is an initial change to address the issue of files or folder being created and deleted. This change only removes from the ModifiedPathsDatabase when the file or folder was created and then delete and the index has not changed in between to solve the issue with builds and other processes creating and deleting a bunch of files and folders.

See #117

sanoursa

First pass through the code. Haven't had a chance to test it yet, hoping to do so by EOD.

GVFS/GVFS.Common/ModifiedPathsDatabase.cs

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

sanoursa · 2018-08-23T15:23:35Z

Any idea why the Mac functional tests failed?

wilbaker · 2018-08-23T18:35:20Z

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

                case FileSystemTask.OperationType.OnFileDeleted:
                    metadata.Add("virtualPath", gitUpdate.VirtualPath);
-                    result = this.AddModifiedPathAndRemoveFromPlaceholderList(gitUpdate.VirtualPath);
+                    if (this.newPaths.Contains(gitUpdate.VirtualPath))


Two related comments:

Currently OnFileDeleted is scheduled from the PreDelete callback on Mac. However, on Mac we can't remove files from ModifiedPaths in PreDelete because we don't actually know that the file has been deleted.

Related to 1 (and something @jrbriggs and I were just discussing), should we try to design this to work on Mac and Windows from the start? Or at this point should we just ensure what whatever we do for Windows does not break Mac?

For this specific feature it might only take a few tweaks to get things working on both platforms (e.g. on Mac we could take an extra step of checking if the file actually was deleted before calling TryRemoveModifiedPath). Thoughts?

CC: @sanoursa for his thoughts as well.

However, on Mac we can't remove files from ModifiedPaths in PreDelete because we don't actually know that the file has been deleted.

The bare minimum change for this PR could be to wrap the new code here in an IsUnderConstruction check.

There's still the open question though of whether we design\implement for both platforms from the start.

I think it would be nice to start thinking about how to make all new features cross-platform from the start. Unless we're up against a part of the Mac design that is still unknown or in heavy flux, I would take the time now to make this work for both.

wilbaker · 2018-08-23T20:30:19Z

@sanoursa I filed #211 for the Mac functional test failures related to mapping.dat in the .gvfsCache folder

wilbaker

Comments so far, I still need to finish reviewing FileSystemCallbacks.cs

GVFS/GVFS.Common/ModifiedPathsDatabase.cs

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/PersistedModifiedPathsTests.cs

wilbaker · 2018-09-05T19:16:53Z

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/PersistedModifiedPathsTests.cs

+        {
+            string tempFolder = this.CreateDirectory(fileSystem, "Temp");
+            string tempFile1 = this.CreateFile(fileSystem, "Temp\\temp1.txt");
+            string tempFile2 = this.CreateFile(fileSystem, "Temp\\temp2.txt");


Use Path.Combine rather than hardcoding \\ so that this will work on Mac as well

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

wilbaker · 2018-09-05T20:50:09Z

GVFS/GVFS.Common/ModifiedPathsDatabase.cs

+            metadata.Add("Area", "ModifiedPathsDatabase");
+            metadata.Add(nameof(entry), entry);
+            metadata.Add(nameof(isFolder), isFolder);
+            metadata.Add("Exception", e.ToString());


What would you think of adding a null check before calling e.ToString()? Most of our other CreateEventMetadata helper methods handle null.

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

wilbaker

Overall looking good, a few outstanding questions on the folder rename scenario.

GVFS/GVFS.FunctionalTests.Windows/Windows/Tests/DiskLayoutUpgradeTests.cs

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

wilbaker · 2018-09-11T16:21:54Z

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

+                                            if (!this.newlyCreatedFileAndFolderPaths.Contains(itemVirtualPath))
                                            {
-                                                relativeFolderPaths.Enqueue(itemVirtualPath);
+                                                this.newlyCreatedFileAndFolderPaths.Add(itemVirtualPath);


It looks like before we were adding itemVirtualPath to ModifiedPaths.dat as well (old line 688):

result = this.TryAddModifiedPath(itemVirtualPath, isFolder: false);

Do we still need to do this?

We are adding the top level folder result = this.TryAddModifiedPath(gitUpdate.VirtualPath, isFolder: true); so we don't need to add all the subfolder and files. This is kind of left over from when we would add the top level folder to that sparse-checkout and then need to add files to the always_exclude. Since this is just one list now we only need the top level folder and git will handle it correctly for both the sparse and exclude cases.

wilbaker · 2018-09-11T16:23:22Z

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

                                            string itemVirtualPath = Path.Combine(folderPath, itemInfo.Name);
-                                            if (itemInfo.IsDirectory)
+                                            string oldItemVirtualPath = gitUpdate.OldVirtualPath + itemVirtualPath.Substring(gitUpdate.VirtualPath.Length);
+                                            if (!this.newlyCreatedFileAndFolderPaths.Contains(itemVirtualPath))


There's no need to call Contains first, we can just call this.newlyCreatedFileAndFolderPaths.Add(itemVirtualPath); because ConcurrentHashSet.Add is really TryAdd:

public bool Add(T entry) { return this.dictionary.TryAdd(entry, true); }

wilbaker · 2018-09-12T21:23:29Z

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

                        if (result == FileSystemTaskResult.Success)
                        {
+                            this.newlyCreatedFileAndFolderPaths.Add(gitUpdate.VirtualPath);
+                            this.TryRemoveModifiedPath(gitUpdate.OldVirtualPath, isFolder: true);


Should we check the return value of TryRemoveModifiedPath here?

I was up in the air about that because on the one hand there won't be any issues if we fail to remove the path except the modified paths doesn't get as small. On the other we could retry when needed and keep the modified paths small but I'm not sure we should kill the mount process if we get a FatalError. What are you thoughts?

I agree there's minimal harm to this failing, but I'm also thinking that if there are failures it could indicate some larger issue with the state of the file system (that I'm not sure we should ignore).

I lean toward checking the value (and killing the process if we see a FatalError). @sanoursa, what are your thoughts?

Yea I don't think we should ever ignore a FatalError. That said, why is TryRemoveModifiedPath ever returning FatalErrors if we don't intend to shut down as a result? It feels like that method shouldn't even be a Try method - if we focus it down to tmp file scenarios, it can just be a best effort and not even return a result.

It is still trying to write to the modified paths database file and the pattern is to retry if there was and IOException and return Fatal if there is some other exception. To keep with the pattern I think we should shut down because we don't know what the state of the modified paths database file is in if we get a fatal error writing to it.

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

wilbaker · 2018-09-12T22:26:27Z

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

+        {
+            if (!this.newlyCreatedFileAndFolderPaths.Contains(virtualPath))
+            {
+                return FileSystemTaskResult.Invalid;


Just noticed this now, but I believe that FileSystemTaskResult.Invalid is never supposed to be used in code (i.e. it's an invalid value for FileSystemTaskResult).

Given the choice I think it would be cleaner to move this check back out of TryRemoveModifiedPath and that way the return value of TryRemoveModifiedPath reflects only whether we succeeded\failed when trying to update ModifiedPaths.

I see that William commented on the Invalid value too, and I agree, from reading this code it seems like this check maybe just doesn't belong in here, at least as currently written.

I would probably change the usage of this method to something like:

if (this.IsTempFile(virtualPath)) { result = this.TryRemoveModifiedPath(virtualPath, isFolder); } else { result = this.TryAddModifiedPath(virtualPath, isFolder); }

sanoursa

The change looks good overall, just needs some cleanup around the pattern for actually removing the entries.

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

sanoursa · 2018-09-14T18:12:14Z

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

+        {
+            if (!this.newlyCreatedFileAndFolderPaths.Contains(virtualPath))
+            {
+                return FileSystemTaskResult.Invalid;


I see that William commented on the Invalid value too, and I agree, from reading this code it seems like this check maybe just doesn't belong in here, at least as currently written.

I would probably change the usage of this method to something like:

if (this.IsTempFile(virtualPath)) { result = this.TryRemoveModifiedPath(virtualPath, isFolder); } else { result = this.TryAddModifiedPath(virtualPath, isFolder); }

sanoursa · 2018-09-14T18:17:03Z

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

                        if (result == FileSystemTaskResult.Success)
                        {
+                            this.newlyCreatedFileAndFolderPaths.Add(gitUpdate.VirtualPath);
+                            this.TryRemoveModifiedPath(gitUpdate.OldVirtualPath, isFolder: true);


Yea I don't think we should ever ignore a FatalError. That said, why is TryRemoveModifiedPath ever returning FatalErrors if we don't intend to shut down as a result? It feels like that method shouldn't even be a Try method - if we focus it down to tmp file scenarios, it can just be a best effort and not even return a result.

sanoursa

Code looks good. Just some perf questions, but feel free to ignore if that makes sense.

sanoursa · 2018-09-20T17:11:42Z

GVFS/GVFS.Common/ModifiedPathsDatabase.cs

+                    return false;
+                }

+                return true;


Remove this line since the very next line is also the same :-)

sanoursa · 2018-09-20T17:15:01Z

GVFS/GVFS.Common/ModifiedPathsDatabase.cs

                    return false;
                }
+
+                return true;


Same here, remove redundant line

GVFS/GVFS.Common/ModifiedPathsDatabase.cs

sanoursa · 2018-09-20T17:29:26Z

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs


        public virtual void OnFileRenamed(string oldRelativePath, string newRelativePath)
        {
+            this.newlyCreatedFileAndFolderPaths.Add(newRelativePath);


Do we actually need this? How often does someone rename into a temp path? If we don't expect benefit from this, I would remove it so that we're not adding unnecessary contention on the concurrent data structure.

This would be so that if something deleted the renamed file we would remove it from the modified paths. Not sure how often something would rename, then delete though.

Yep I get that that's the scenario it's supporting. What I don't know is how often that actually happens, but my guess is it's pretty unlikely. We know that we're adding lock contention by including this line, so we should only do that if we can also measure an actual benefit.

I will remove this for now because we aren't measuring how often this is happening so we can't measure any benefit if we don't know what or if there are current difficulties that are from this.

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

wilbaker

Latest commit looks good, just a minor suggestion on adding some comment(s)

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs

…paths

sanoursa reviewed Aug 23, 2018

View reviewed changes

wilbaker reviewed Aug 23, 2018

View reviewed changes

kewillford force-pushed the cleanup_modifiedpaths branch 2 times, most recently from 241eea9 to 259e056 Compare August 30, 2018 18:46

wilbaker reviewed Sep 5, 2018

View reviewed changes

kewillford force-pushed the cleanup_modifiedpaths branch 2 times, most recently from b1b3d0e to bc005ee Compare September 5, 2018 20:37

wilbaker reviewed Sep 5, 2018

View reviewed changes

wilbaker reviewed Sep 11, 2018

View reviewed changes

kewillford force-pushed the cleanup_modifiedpaths branch 2 times, most recently from 59d5580 to 39f9448 Compare September 12, 2018 15:27

wilbaker reviewed Sep 12, 2018

View reviewed changes

sanoursa suggested changes Sep 14, 2018

View reviewed changes

kewillford force-pushed the cleanup_modifiedpaths branch 2 times, most recently from 7618ea2 to a6ba326 Compare September 20, 2018 14:46

wilbaker approved these changes Sep 20, 2018

View reviewed changes

sanoursa approved these changes Sep 20, 2018

View reviewed changes

kewillford force-pushed the cleanup_modifiedpaths branch from 72472d8 to 8b16562 Compare September 21, 2018 19:42

wilbaker approved these changes Sep 24, 2018

View reviewed changes

GVFS/GVFS.Virtualization/FileSystemCallbacks.cs Show resolved Hide resolved

Kevin Willford added 8 commits September 25, 2018 17:31

Add tests for removing entries from the modified files database

3f8fd9e

Add TryRemove method to the ModifiedPathsDatabase

a0f9b40

Handle the file system callbacks to track created then deleted files

ae5f9ba

Add handling of rename of files for modified paths cleanup

b50dd1a

Handle cleaning up modified paths when a folder is renamed

848e6d3

Add null check when creating trace metadata using and exception

c189b2c

Fix broken tests

1772519

Simplify checking the newly created files and removing from modified …

9f8944e

…paths

Kevin Willford added 6 commits September 25, 2018 17:31

Fix broken tests for cleaning modified paths

f7e268a

Remove extra return statments and adding to new paths during rename

a0023aa

Add PreDelete handler in BG operation to handle Mac

dcf925f

Add comment explaining only one of predelete or delete should be queued

45159db

Fix case only folder rename test

472904c

Fix test that moved new folder outside the repo

243f621

kewillford force-pushed the cleanup_modifiedpaths branch from 19f3acd to 243f621 Compare September 25, 2018 23:32

kewillford merged commit 08ee71a into microsoft:master Sep 26, 2018

kewillford deleted the cleanup_modifiedpaths branch September 26, 2018 18:46

wilbaker mentioned this pull request Oct 8, 2018

Mac: Deliver oldPath to VFS for Git for rename notifications #356

Closed

jrbriggs mentioned this pull request Oct 12, 2018

Merge milestones/M142 to releases/shipped #381

Merged

Conversation

kewillford commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanoursa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanoursa commented Aug 23, 2018

Uh oh!

wilbaker Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilbaker commented Aug 23, 2018

Uh oh!

wilbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wilbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wilbaker Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanoursa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanoursa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kewillford commented Aug 23, 2018 •

edited

Loading

wilbaker Aug 23, 2018 •

edited

Loading

wilbaker Sep 12, 2018 •

edited

Loading