[Lldb-commits] [PATCH] D151765: [lldb] Introduce the FileSpecBuilder abstraction

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

The idea behind FileSpec containing two ConstStrings, one for the directory and 
one for he filename is for lookup performance when searching thousands of line 
tables for a given file and line when setting breakpoints.

Currently we pass in a FileSpec + line number and we expect fast searching. 
This is currently done by asking each CompileUnit to find a matching file in 
its support files array and to return a matching index. If just the filename is 
filled in, then we compare the just the ConstString for m_filename which is 
really quick for ConstString values. Then we compare all or part of the 
m_directory if the path is a full path or if it is relative. But at least the 
filename comparison is very fast. If we get a valid index back, then we know to 
search the compile unit's full line table for any matching entries by looking 
for any line entries with a matching file index. IF the file index is invalid, 
we don't materialize the line table at all for a compile unit.

Maybe we want the notion of a "FileSpec" (which could contain a llvm::Vector 
class) and a ConstFileSpec (which would contain two ConstString objects like 
FileSpec currently has). And we make anyone that needs quick searching, like 
the support files for a compile unit, use the new ConstFileSpec class.

If you change anything in the FileSpec class, please test setting breakpoints 
with a really large codebase to ensure we don't regress performance.

If we go the FileSpecBuilder route, we probably want to remove all FileSpec 
path modification methods and force people to use this class by converting a 
FileSpec to a FileSpecBuilder, manipulating the path with the FileSpecBuilder 
calls, and then extracting the FileSpec object from the FileSpecBuilder at the 
end.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151765/new/

https://reviews.llvm.org/D151765

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D151765: [lldb] Introduce the FileSpecBuilder abstraction

[Alex Langford via Phabricator via lldb-commits]

bulbazord planned changes to this revision.
bulbazord added a comment.

In D151765#4385711 , @jingham wrote:

> Why did you choose to have a separate FileSpecBuilder class, rather than 
> making FileSpec smarter about the structure of the path (e.g. have an array 
> of StringRef's into the paths for each component.)   We could do the parse 
> once the first time a path element was requested, and then operations like 
> "RemovePathComponent" would be trivial.  We could maybe even get rid of the 
> distinction between "path" and "filename" since "filename" is really just 
> "last path component".

You mean have FileSpec replace its `ConstString m_directory` and `ConstString 
m_filename` fields with `llvm::SmallString m_path`? That would indeed avoid 
something like `FileSpecBuilder` (and probably be simpler in the end). As long 
as the API of FileSpec stays the same, the changes to call sites should be 
minimal. The thing I'm mostly concerned about is lifetimes... How often do we 
grab a ConstString from a FileSpec and not think about the lifetime? Hopefully 
not many places.

I'll try this out and see what the fallout is. If it's the better way to go, 
then I'll just refactor all the places where those assumptions are problematic.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151765/new/

https://reviews.llvm.org/D151765

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D151765: [lldb] Introduce the FileSpecBuilder abstraction

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Why did you choose to have a separate FileSpecBuilder class, rather than making 
FileSpec smarter about the structure of the path (e.g. have an array of 
StringRef's into the paths for each component.)   We could do the parse once 
the first time a path element was requested, and then operations like 
"RemovePathComponent" would be trivial.  We could maybe even get rid of the 
distinction between "path" and "filename" since "filename" is really just "last 
path component".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151765/new/

https://reviews.llvm.org/D151765

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D151765: [lldb] Introduce the FileSpecBuilder abstraction

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: JDevlieghere, mib, jingham, jasonmolenda, clayborg.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Mutating a FileSpec is quite expensive. Every time you add or remove a
component of a path, we need to re-parse the entire path and insert a
new directory and filename into the ConstString StringPool. In many
cases, we take an existing FilePath and slowly morph it into the one we
want to actually use.

In order to improve performance, I want to introduce a new abstraction
called the `FileSpecBuilder`. The idea is that it maintains a
SmallString to store the entire path and a path style. It uses llvm's
path manipulation code to quickly append and remove things so we don't
have to re-parse the path every single time. When you're done
manipulating paths and want a FileSpec for sure, you can invoke
`FileSpecBuilder::CreateFileSpec` to create a new FileSpec, only parsing
the path once.

This patch primarily is to gather feedback about the idea. I wanted to
keep this patch small and targeted to get specific feedback, and for
that reason I avoided doing any kind of refactoring as most changes will
be quite invasive. Instead I opted to add unit tests to show how I want
FileSpecBuilder to be used in code. If this abstraction goes into LLDB,
I will refactor every place where we mutate a FileSpec until
FileSpecBuilder is the default way of manipulating paths.

For further motivation of this change, please see:
https://reviews.llvm.org/D149096


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151765

Files:
  lldb/include/lldb/Utility/FileSpecBuilder.h
  lldb/source/Utility/CMakeLists.txt
  lldb/source/Utility/FileSpecBuilder.cpp
  lldb/unittests/Utility/FileSpecBuilderTest.cpp

Index: lldb/unittests/Utility/FileSpecBuilderTest.cpp
===
--- /dev/null
+++ lldb/unittests/Utility/FileSpecBuilderTest.cpp
@@ -0,0 +1,118 @@
+//===-- FileSpecBuilderTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/FileSpecBuilder.h"
+
+using namespace lldb_private;
+
+static FileSpec PosixSpec(llvm::StringRef path) {
+  return FileSpec(path, FileSpec::Style::posix);
+}
+
+static FileSpec WindowsSpec(llvm::StringRef path) {
+  return FileSpec(path, FileSpec::Style::windows);
+}
+
+TEST(FileSpecBuilderTest, AppendPathComponent) {
+  FileSpecBuilder from_scratch_posix("/", FileSpecBuilder::Style::posix);
+  EXPECT_EQ("/", from_scratch_posix.GetCurrentPath());
+  from_scratch_posix.AppendPathComponent("foo");
+  EXPECT_EQ("/foo", from_scratch_posix.GetCurrentPath());
+  from_scratch_posix.AppendPathComponent("bar");
+  EXPECT_EQ("/foo/bar", from_scratch_posix.GetCurrentPath());
+  from_scratch_posix.AppendPathComponent("baz");
+  EXPECT_EQ("/foo/bar/baz", from_scatch_posix.GetCurrentPath());
+  const FileSpec posix_scratch_result = from_scratch_posix.CreateFileSpec();
+  EXPECT_STREQ("/foo/bar/baz", posix_scratch_result.GetPath().c_str());
+
+  const FileSpec fs_posix = PosixSpec("/foo");
+  FileSpecBuilder builder_posix(fs_posix);
+  builder_posix.AppendPathComponent("bar");
+  EXPECT_EQ("/foo/bar", builder_posix.GetCurrentPath());
+  const FileSpec posix_result = builder_posix.CreateFileSpec();
+  EXPECT_STREQ("/foo/bar", posix_result.GetPath().c_str());
+
+  const FileSpec fs_windows = WindowsSpec("F:\\");
+  FileSpecBuilder builder_windows(fs_windows);
+  builder_windows.AppendPathComponent("bar");
+  EXPECT_EQ("F:\\bar", builder_windows.GetCurrentPath());
+  builder_windows.AppendPathComponent("baz");
+  EXPECT_EQ("F:\\bar\\baz", builder_windows.GetCurrentPath());
+  const FileSpec windows_result = builder_windows.CreateFileSpec();
+  EXPECT_STREQ("F:\\bar\\baz", windows_result.GetPath().c_str());
+}
+
+TEST(FileSpecBuilderTest, RemoveLastPathComponent) {
+  const FileSpec fs_posix = PosixSpec("/foo/bar/baz");
+  FileSpecBuilder builder_posix(fs_posix);
+
+  EXPECT_EQ("/foo/bar/baz", builder_posix.GetCurrentPath());
+  EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+  EXPECT_EQ("/foo/bar", builder_posix.GetCurrentPath());
+  EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+  EXPECT_EQ("/foo", builder_posix.GetCurrentPath());
+  EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+  EXPECT_EQ("/", builder_posix.GetCurrentPath());
+  EXPECT_FALSE(fs_posix.RemoveLastPathComponent());
+  EXPECT_EQ("/", builder_posix.GetCurrentPath());
+
+  const FileSpec fs_posix_relative = PosixSpec("./foo/bar/baz");
+  FileSpecBuilder builder_