r269100 - [VFS] Reconstruct the VFS overlay tree for more accurate lookup
Bruno Cardoso Lopes via cfe-commits
cfe-commits at lists.llvm.org
Tue May 10 21:49:43 PDT 2016
Thanks Sean,
I was about to ask for help and try a 4th attempt, as you noted after
3 attempts I was not yet able to make this work on windows though...
The new tests I added first failed on windows because looks like a
missing "/" on the dir name forces "\\" to be generated while
appending the filename instead of "/". After fixing that, now seems
like the directory iteration on windows has a different order from
what we are getting on the other platforms, do you know anything about
this? Or can you help me debug it once I land it again? I might
check-in the new tests separately from the rest to make it easier to
revert.
******************** TEST 'Clang-Unit ::
Basic/BasicTests.exe/VFSFromYAMLTest.DirectoryIteration' FAILED
********************
Note: Google Test filter = VFSFromYAMLTest.DirectoryIteration
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from VFSFromYAMLTest
[ RUN ] VFSFromYAMLTest.DirectoryIteration
C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\unittests\Basic\VirtualFileSystemTest.cpp(379):
error: Value of: I->getName()
Actual: { '/' (47, 0x2F), '/' (47, 0x2F), 'r' (114, 0x72), 'o' (111,
0x6F), 'o' (111, 0x6F), 't' (116, 0x74), '/' (47, 0x2F), 'b' (98,
0x62), 'a' (97, 0x61), 'z' (122, 0x7A), '/' (47, 0x2F), 'c' (99, 0x63)
}
Expected: *ExpectedIter
Which is: { '/' (47, 0x2F), '/' (47, 0x2F), 'r' (114, 0x72), 'o' (111,
0x6F), 'o' (111, 0x6F), 't' (116, 0x74), '/' (47, 0x2F), 'b' (98,
0x62), 'a' (97, 0x61), 'z' (122, 0x7A), '/' (47, 0x2F), 'x' (120,
0x78) }
C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\unittests\Basic\VirtualFileSystemTest.cpp(381):
error: Value of: ExpectedIter
Actual: 000000C72A87F7E0
Expected: ExpectedEnd
Which is: 000000C72A87F800
[ FAILED ] VFSFromYAMLTest.DirectoryIteration (1 ms)
On Tue, May 10, 2016 at 9:27 PM, Sean Silva <chisophugis at gmail.com> wrote:
> Hi Bruno,
>
> I had to revert this in r269100 because it was looking like the bot was
> going to be left red overnight.
>
> Changes to this VFS code seem to have a trend of breaking on windows. Any
> idea why that is? I can understand things breaking on windows when writing
> low-level parts of an FS abstraction, but this patch seems fairly
> high-level. Is there a missing layering or something?
>
> -- Sean Silva
>
> On Tue, May 10, 2016 at 11:43 AM, Bruno Cardoso Lopes via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>>
>> Author: bruno
>> Date: Tue May 10 13:43:00 2016
>> New Revision: 269100
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=269100&view=rev
>> Log:
>> [VFS] Reconstruct the VFS overlay tree for more accurate lookup
>>
>> The way we currently build the internal VFS overlay representation leads
>> to inefficient path search and might yield wrong answers when asked for
>> recursive or regular directory iteration.
>>
>> Currently, when reading an YAML file, each YAML root entry is placed
>> inside a new root in the filesystem overlay. In the crash reproducer, a
>> simple "@import Foundation" currently maps to 43 roots, and when looking
>> up paths, we traverse a directory tree for each of these different
>> roots, until we find a match (or don't). This has two consequences:
>>
>> - It's slow.
>> - Directory iteration gives incomplete results since it only return
>> results within one root - since contents of the same directory can be
>> declared inside different roots, the result isn't accurate.
>>
>> This is in part fault of the way we currently write out the YAML file
>> when emitting the crash reproducer - we could generate only one root and
>> that would make it fast and correct again. However, we should not rely
>> on how the client writes the YAML, but provide a good internal
>> representation regardless.
>>
>> This patch builds a proper virtual directory tree out of the YAML
>> representation, allowing faster search and proper iteration. Besides the
>> crash reproducer, this potentially benefits other VFS clients.
>>
>> Modified:
>> cfe/trunk/lib/Basic/VirtualFileSystem.cpp
>> cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
>>
>> Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=269100&r1=269099&r2=269100&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
>> +++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Tue May 10 13:43:00 2016
>> @@ -719,7 +719,13 @@ public:
>> Status S)
>> : Entry(EK_Directory, Name), Contents(std::move(Contents)),
>> S(std::move(S)) {}
>> + RedirectingDirectoryEntry(StringRef Name, Status S)
>> + : Entry(EK_Directory, Name), S(std::move(S)) {}
>> Status getStatus() { return S; }
>> + void addContent(std::unique_ptr<Entry> Content) {
>> + Contents.push_back(std::move(Content));
>> + }
>> + Entry *getLastContent() const { return Contents.back().get(); }
>> typedef decltype(Contents)::iterator iterator;
>> iterator contents_begin() { return Contents.begin(); }
>> iterator contents_end() { return Contents.end(); }
>> @@ -747,6 +753,7 @@ public:
>> return UseName == NK_NotSet ? GlobalUseExternalName
>> : (UseName == NK_External);
>> }
>> + NameKind getUseName() const { return UseName; }
>> static bool classof(const Entry *E) { return E->getKind() == EK_File; }
>> };
>>
>> @@ -1023,6 +1030,70 @@ class RedirectingFileSystemParser {
>> return true;
>> }
>>
>> + Entry *lookupOrCreateEntry(RedirectingFileSystem *FS, StringRef Name,
>> + Entry *ParentEntry = nullptr) {
>> + if (!ParentEntry) { // Look for a existent root
>> + for (const std::unique_ptr<Entry> &Root : FS->Roots) {
>> + if (Name.equals(Root->getName())) {
>> + ParentEntry = Root.get();
>> + return ParentEntry;
>> + }
>> + }
>> + } else { // Advance to the next component
>> + auto *DE = dyn_cast<RedirectingDirectoryEntry>(ParentEntry);
>> + for (std::unique_ptr<Entry> &Content :
>> + llvm::make_range(DE->contents_begin(), DE->contents_end())) {
>> + auto *DirContent =
>> dyn_cast<RedirectingDirectoryEntry>(Content.get());
>> + if (DirContent && Name.equals(Content->getName()))
>> + return DirContent;
>> + }
>> + }
>> +
>> + // ... or create a new one
>> + std::unique_ptr<Entry> E =
>> llvm::make_unique<RedirectingDirectoryEntry>(
>> + Name, Status("", getNextVirtualUniqueID(), sys::TimeValue::now(),
>> 0, 0,
>> + 0, file_type::directory_file, sys::fs::all_all));
>> +
>> + if (!ParentEntry) { // Add a new root to the overlay
>> + FS->Roots.push_back(std::move(E));
>> + ParentEntry = FS->Roots.back().get();
>> + return ParentEntry;
>> + }
>> +
>> + auto *DE = dyn_cast<RedirectingDirectoryEntry>(ParentEntry);
>> + DE->addContent(std::move(E));
>> + return DE->getLastContent();
>> + }
>> +
>> + void uniqueOverlayTree(RedirectingFileSystem *FS, Entry *SrcE,
>> + Entry *NewParentE = nullptr) {
>> + StringRef Name = SrcE->getName();
>> + switch (SrcE->getKind()) {
>> + case EK_Directory: {
>> + auto *DE = dyn_cast<RedirectingDirectoryEntry>(SrcE);
>> + assert(DE && "Must be a directory");
>> + // Empty directories could be present in the YAML as a way to
>> + // describe a file for a current directory after some of its subdir
>> + // is parsed. This only leads to redundant walks, ignore it.
>> + if (!Name.empty())
>> + NewParentE = lookupOrCreateEntry(FS, Name, NewParentE);
>> + for (std::unique_ptr<Entry> &SubEntry :
>> + llvm::make_range(DE->contents_begin(), DE->contents_end()))
>> + uniqueOverlayTree(FS, SubEntry.get(), NewParentE);
>> + break;
>> + }
>> + case EK_File: {
>> + auto *FE = dyn_cast<RedirectingFileEntry>(SrcE);
>> + assert(FE && "Must be a file");
>> + assert(NewParentE && "Parent entry must exist");
>> + auto *DE = dyn_cast<RedirectingDirectoryEntry>(NewParentE);
>> + DE->addContent(llvm::make_unique<RedirectingFileEntry>(
>> + Name, FE->getExternalContentsPath(), FE->getUseName()));
>> + break;
>> + }
>> + }
>> + }
>> +
>> std::unique_ptr<Entry> parseEntry(yaml::Node *N, RedirectingFileSystem
>> *FS) {
>> yaml::MappingNode *M = dyn_cast<yaml::MappingNode>(N);
>> if (!M) {
>> @@ -1225,6 +1296,7 @@ public:
>> };
>>
>> DenseMap<StringRef, KeyStatus> Keys(std::begin(Fields),
>> std::end(Fields));
>> + std::vector<std::unique_ptr<Entry>> RootEntries;
>>
>> // Parse configuration and 'roots'
>> for (yaml::MappingNode::iterator I = Top->begin(), E = Top->end(); I
>> != E;
>> @@ -1247,7 +1319,7 @@ public:
>> for (yaml::SequenceNode::iterator I = Roots->begin(), E =
>> Roots->end();
>> I != E; ++I) {
>> if (std::unique_ptr<Entry> E = parseEntry(&*I, FS))
>> - FS->Roots.push_back(std::move(E));
>> + RootEntries.push_back(std::move(E));
>> else
>> return false;
>> }
>> @@ -1288,6 +1360,13 @@ public:
>>
>> if (!checkMissingKeys(Top, Keys))
>> return false;
>> +
>> + // Now that we sucessefully parsed the YAML file, canonicalize the
>> internal
>> + // representation to a proper directory tree so that we can search
>> faster
>> + // inside the VFS.
>> + for (std::unique_ptr<Entry> &E : RootEntries)
>> + uniqueOverlayTree(FS, E.get());
>> +
>> return true;
>> }
>> };
>>
>> Modified: cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp?rev=269100&r1=269099&r2=269100&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp (original)
>> +++ cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp Tue May 10
>> 13:43:00 2016
>> @@ -1022,9 +1022,14 @@ TEST_F(VFSFromYAMLTest, DirectoryIterati
>> Lower->addDirectory("//root/");
>> Lower->addDirectory("//root/foo");
>> Lower->addDirectory("//root/foo/bar");
>> + Lower->addDirectory("//root/zab");
>> + Lower->addDirectory("//root/baz");
>> Lower->addRegularFile("//root/foo/bar/a");
>> Lower->addRegularFile("//root/foo/bar/b");
>> Lower->addRegularFile("//root/file3");
>> + Lower->addRegularFile("//root/zab/a");
>> + Lower->addRegularFile("//root/zab/b");
>> + Lower->addRegularFile("//root/baz/c");
>> IntrusiveRefCntPtr<vfs::FileSystem> FS =
>> getFromYAMLString("{ 'use-external-names': false,\n"
>> " 'roots': [\n"
>> @@ -1042,6 +1047,26 @@ TEST_F(VFSFromYAMLTest, DirectoryIterati
>> " 'external-contents':
>> '//root/foo/bar/b'\n"
>> " }\n"
>> " ]\n"
>> + "},\n"
>> + "{\n"
>> + " 'type': 'directory',\n"
>> + " 'name': '//root/baz',\n"
>> + " 'contents': [ {\n"
>> + " 'type': 'file',\n"
>> + " 'name': 'x',\n"
>> + " 'external-contents':
>> '//root/zab/a'\n"
>> + " }\n"
>> + " ]\n"
>> + "},\n"
>> + "{\n"
>> + " 'type': 'directory',\n"
>> + " 'name': '//root/baz',\n"
>> + " 'contents': [ {\n"
>> + " 'type': 'file',\n"
>> + " 'name': 'y',\n"
>> + " 'external-contents':
>> '//root/zab/b'\n"
>> + " }\n"
>> + " ]\n"
>> "}\n"
>> "]\n"
>> "}",
>> @@ -1054,8 +1079,12 @@ TEST_F(VFSFromYAMLTest, DirectoryIterati
>>
>> std::error_code EC;
>> checkContents(O->dir_begin("//root/", EC),
>> - {"//root/file1", "//root/file2", "//root/file3",
>> "//root/foo"});
>> + {"//root/file1", "//root/file2", "//root/baz",
>> "//root/file3",
>> + "//root/foo", "//root/zab"});
>>
>> checkContents(O->dir_begin("//root/foo/bar", EC),
>> {"//root/foo/bar/a", "//root/foo/bar/b"});
>> +
>> + checkContents(O->dir_begin("//root/baz", EC),
>> + {"//root/baz/x", "//root/baz/y", "//root/baz/c"});
>> }
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
--
Bruno Cardoso Lopes
http://www.brunocardoso.cc
More information about the cfe-commits
mailing list