[Lldb-commits] [PATCH] D153834: [lldb] Add two-level caching in the source manager

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG51944e78bb37: [lldb] Add two-level caching in the source 
manager (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153834

Files:
  lldb/include/lldb/Core/SourceManager.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Utility/LLDBLog.h
  lldb/source/Commands/CommandObjectSource.cpp
  lldb/source/Core/SourceManager.cpp
  lldb/source/Utility/LLDBLog.cpp
  lldb/test/API/source-manager/TestSourceManager.py
  lldb/unittests/Core/SourceManagerTest.cpp

Index: lldb/unittests/Core/SourceManagerTest.cpp
===
--- lldb/unittests/Core/SourceManagerTest.cpp
+++ lldb/unittests/Core/SourceManagerTest.cpp
@@ -30,7 +30,7 @@
   // Insert: foo
   FileSpec foo_file_spec("foo");
   auto foo_file_sp =
-  std::make_shared(foo_file_spec, nullptr);
+  std::make_shared(foo_file_spec, lldb::DebuggerSP());
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
 
   // Query: foo, expect found.
@@ -44,7 +44,7 @@
   // Insert: foo
   FileSpec foo_file_spec("foo");
   auto foo_file_sp =
-  std::make_shared(foo_file_spec, nullptr);
+  std::make_shared(foo_file_spec, lldb::DebuggerSP());
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
 
   // Query: bar, expect not found.
@@ -62,8 +62,8 @@
   FileSystem::Instance().Resolve(resolved_foo_file_spec);
 
   // Create the file with the resolved file spec.
-  auto foo_file_sp =
-  std::make_shared(resolved_foo_file_spec, nullptr);
+  auto foo_file_sp = std::make_shared(
+  resolved_foo_file_spec, lldb::DebuggerSP());
 
   // Cache the result with the unresolved file spec.
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
Index: lldb/test/API/source-manager/TestSourceManager.py
===
--- lldb/test/API/source-manager/TestSourceManager.py
+++ lldb/test/API/source-manager/TestSourceManager.py
@@ -10,6 +10,7 @@
 """
 
 import os
+import io
 import stat
 
 import lldb
@@ -37,6 +38,33 @@
 self.file = self.getBuildArtifact("main-copy.c")
 self.line = line_number("main.c", "// Set break point at this line.")
 
+def modify_content(self):
+
+# Read the main.c file content.
+with io.open(self.file, "r", newline="\n") as f:
+original_content = f.read()
+if self.TraceOn():
+print("original content:", original_content)
+
+# Modify the in-memory copy of the original source code.
+new_content = original_content.replace("Hello world", "Hello lldb", 1)
+
+# Modify the source code file.
+# If the source was read only, the copy will also be read only.
+# Run "chmod u+w" on it first so we can modify it.
+statinfo = os.stat(self.file)
+os.chmod(self.file, statinfo.st_mode | stat.S_IWUSR)
+
+with io.open(self.file, "w", newline="\n") as f:
+time.sleep(1)
+f.write(new_content)
+if self.TraceOn():
+print("new content:", new_content)
+print(
+"os.path.getmtime() after writing new content:",
+os.path.getmtime(self.file),
+)
+
 def get_expected_stop_column_number(self):
 """Return the 1-based column number of the first non-whitespace
 character in the breakpoint source line."""
@@ -234,32 +262,20 @@
 self.fail("Fail to display source level breakpoints")
 self.assertTrue(int(m.group(1)) > 0)
 
-# Read the main.c file content.
-with io.open(self.file, "r", newline="\n") as f:
-original_content = f.read()
-if self.TraceOn():
-print("original content:", original_content)
-
-# Modify the in-memory copy of the original source code.
-new_content = original_content.replace("Hello world", "Hello lldb", 1)
+# Modify content
+self.modify_content()
 
-# Modify the source code file.
-# If the source was read only, the copy will also be read only.
-# Run "chmod u+w" on it first so we can modify it.
-statinfo = os.stat(self.file)
-os.chmod(self.file, statinfo.st_mode | stat.S_IWUSR)
+# Display the source code again. We should not see the updated line.
+self.expect(
+"source list -f main-copy.c -l %d" % self.line,
+SOURCE_DISPLAYED_CORRECTLY,
+substrs=["Hello world"],
+)
 
-with io.open(self.file, "w", newline="\n") as f:
-time.sleep(1)
-f.write(new_content)
-if self.TraceOn():
-print("new content:", new_content)
-print(
-"os.path.getmtime() after writing new content:",
-  

[Lldb-commits] [PATCH] D153834: [lldb] Add two-level caching in the source manager

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D153834

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

[Lldb-commits] [PATCH] D153834: [lldb] Add two-level caching in the source manager

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

This looks like a good change to me.  I don't have any comments over Alex's 
feedback.


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

https://reviews.llvm.org/D153834

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

[Lldb-commits] [PATCH] D153834: [lldb] Add two-level caching in the source manager

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 535136.
JDevlieghere marked 2 inline comments as done.

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

https://reviews.llvm.org/D153834

Files:
  lldb/include/lldb/Core/SourceManager.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Utility/LLDBLog.h
  lldb/source/Commands/CommandObjectSource.cpp
  lldb/source/Core/SourceManager.cpp
  lldb/source/Utility/LLDBLog.cpp
  lldb/test/API/source-manager/TestSourceManager.py
  lldb/unittests/Core/SourceManagerTest.cpp

Index: lldb/unittests/Core/SourceManagerTest.cpp
===
--- lldb/unittests/Core/SourceManagerTest.cpp
+++ lldb/unittests/Core/SourceManagerTest.cpp
@@ -30,7 +30,7 @@
   // Insert: foo
   FileSpec foo_file_spec("foo");
   auto foo_file_sp =
-  std::make_shared(foo_file_spec, nullptr);
+  std::make_shared(foo_file_spec, lldb::DebuggerSP());
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
 
   // Query: foo, expect found.
@@ -44,7 +44,7 @@
   // Insert: foo
   FileSpec foo_file_spec("foo");
   auto foo_file_sp =
-  std::make_shared(foo_file_spec, nullptr);
+  std::make_shared(foo_file_spec, lldb::DebuggerSP());
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
 
   // Query: bar, expect not found.
@@ -62,8 +62,8 @@
   FileSystem::Instance().Resolve(resolved_foo_file_spec);
 
   // Create the file with the resolved file spec.
-  auto foo_file_sp =
-  std::make_shared(resolved_foo_file_spec, nullptr);
+  auto foo_file_sp = std::make_shared(
+  resolved_foo_file_spec, lldb::DebuggerSP());
 
   // Cache the result with the unresolved file spec.
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
Index: lldb/test/API/source-manager/TestSourceManager.py
===
--- lldb/test/API/source-manager/TestSourceManager.py
+++ lldb/test/API/source-manager/TestSourceManager.py
@@ -10,6 +10,7 @@
 """
 
 import os
+import io
 import stat
 
 import lldb
@@ -37,6 +38,33 @@
 self.file = self.getBuildArtifact("main-copy.c")
 self.line = line_number("main.c", "// Set break point at this line.")
 
+def modify_content(self):
+
+# Read the main.c file content.
+with io.open(self.file, "r", newline="\n") as f:
+original_content = f.read()
+if self.TraceOn():
+print("original content:", original_content)
+
+# Modify the in-memory copy of the original source code.
+new_content = original_content.replace("Hello world", "Hello lldb", 1)
+
+# Modify the source code file.
+# If the source was read only, the copy will also be read only.
+# Run "chmod u+w" on it first so we can modify it.
+statinfo = os.stat(self.file)
+os.chmod(self.file, statinfo.st_mode | stat.S_IWUSR)
+
+with io.open(self.file, "w", newline="\n") as f:
+time.sleep(1)
+f.write(new_content)
+if self.TraceOn():
+print("new content:", new_content)
+print(
+"os.path.getmtime() after writing new content:",
+os.path.getmtime(self.file),
+)
+
 def get_expected_stop_column_number(self):
 """Return the 1-based column number of the first non-whitespace
 character in the breakpoint source line."""
@@ -234,32 +262,20 @@
 self.fail("Fail to display source level breakpoints")
 self.assertTrue(int(m.group(1)) > 0)
 
-# Read the main.c file content.
-with io.open(self.file, "r", newline="\n") as f:
-original_content = f.read()
-if self.TraceOn():
-print("original content:", original_content)
-
-# Modify the in-memory copy of the original source code.
-new_content = original_content.replace("Hello world", "Hello lldb", 1)
+# Modify content
+self.modify_content()
 
-# Modify the source code file.
-# If the source was read only, the copy will also be read only.
-# Run "chmod u+w" on it first so we can modify it.
-statinfo = os.stat(self.file)
-os.chmod(self.file, statinfo.st_mode | stat.S_IWUSR)
+# Display the source code again. We should not see the updated line.
+self.expect(
+"source list -f main-copy.c -l %d" % self.line,
+SOURCE_DISPLAYED_CORRECTLY,
+substrs=["Hello world"],
+)
 
-with io.open(self.file, "w", newline="\n") as f:
-time.sleep(1)
-f.write(new_content)
-if self.TraceOn():
-print("new content:", new_content)
-print(
-"os.path.getmtime() after writing new content:",
-os.path.getmtime(self.file),
-)
+# clear the source cache.
+self.runCmd("source cache clear")
 
-# D

[Lldb-commits] [PATCH] D153834: [lldb] Add two-level caching in the source manager

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 535085.
JDevlieghere added a comment.

Fix boolean algebra


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

https://reviews.llvm.org/D153834

Files:
  lldb/include/lldb/Core/SourceManager.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Utility/LLDBLog.h
  lldb/source/Commands/CommandObjectSource.cpp
  lldb/source/Core/SourceManager.cpp
  lldb/source/Utility/LLDBLog.cpp
  lldb/test/API/source-manager/TestSourceManager.py
  lldb/unittests/Core/SourceManagerTest.cpp

Index: lldb/unittests/Core/SourceManagerTest.cpp
===
--- lldb/unittests/Core/SourceManagerTest.cpp
+++ lldb/unittests/Core/SourceManagerTest.cpp
@@ -30,7 +30,7 @@
   // Insert: foo
   FileSpec foo_file_spec("foo");
   auto foo_file_sp =
-  std::make_shared(foo_file_spec, nullptr);
+  std::make_shared(foo_file_spec, lldb::DebuggerSP());
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
 
   // Query: foo, expect found.
@@ -44,7 +44,7 @@
   // Insert: foo
   FileSpec foo_file_spec("foo");
   auto foo_file_sp =
-  std::make_shared(foo_file_spec, nullptr);
+  std::make_shared(foo_file_spec, lldb::DebuggerSP());
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
 
   // Query: bar, expect not found.
@@ -62,8 +62,8 @@
   FileSystem::Instance().Resolve(resolved_foo_file_spec);
 
   // Create the file with the resolved file spec.
-  auto foo_file_sp =
-  std::make_shared(resolved_foo_file_spec, nullptr);
+  auto foo_file_sp = std::make_shared(
+  resolved_foo_file_spec, lldb::DebuggerSP());
 
   // Cache the result with the unresolved file spec.
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
Index: lldb/test/API/source-manager/TestSourceManager.py
===
--- lldb/test/API/source-manager/TestSourceManager.py
+++ lldb/test/API/source-manager/TestSourceManager.py
@@ -10,6 +10,7 @@
 """
 
 import os
+import io
 import stat
 
 import lldb
@@ -37,6 +38,33 @@
 self.file = self.getBuildArtifact("main-copy.c")
 self.line = line_number("main.c", "// Set break point at this line.")
 
+def modify_content(self):
+
+# Read the main.c file content.
+with io.open(self.file, "r", newline="\n") as f:
+original_content = f.read()
+if self.TraceOn():
+print("original content:", original_content)
+
+# Modify the in-memory copy of the original source code.
+new_content = original_content.replace("Hello world", "Hello lldb", 1)
+
+# Modify the source code file.
+# If the source was read only, the copy will also be read only.
+# Run "chmod u+w" on it first so we can modify it.
+statinfo = os.stat(self.file)
+os.chmod(self.file, statinfo.st_mode | stat.S_IWUSR)
+
+with io.open(self.file, "w", newline="\n") as f:
+time.sleep(1)
+f.write(new_content)
+if self.TraceOn():
+print("new content:", new_content)
+print(
+"os.path.getmtime() after writing new content:",
+os.path.getmtime(self.file),
+)
+
 def get_expected_stop_column_number(self):
 """Return the 1-based column number of the first non-whitespace
 character in the breakpoint source line."""
@@ -234,32 +262,20 @@
 self.fail("Fail to display source level breakpoints")
 self.assertTrue(int(m.group(1)) > 0)
 
-# Read the main.c file content.
-with io.open(self.file, "r", newline="\n") as f:
-original_content = f.read()
-if self.TraceOn():
-print("original content:", original_content)
-
-# Modify the in-memory copy of the original source code.
-new_content = original_content.replace("Hello world", "Hello lldb", 1)
+# Modify content
+self.modify_content()
 
-# Modify the source code file.
-# If the source was read only, the copy will also be read only.
-# Run "chmod u+w" on it first so we can modify it.
-statinfo = os.stat(self.file)
-os.chmod(self.file, statinfo.st_mode | stat.S_IWUSR)
+# Display the source code again. We should not see the updated line.
+self.expect(
+"source list -f main-copy.c -l %d" % self.line,
+SOURCE_DISPLAYED_CORRECTLY,
+substrs=["Hello world"],
+)
 
-with io.open(self.file, "w", newline="\n") as f:
-time.sleep(1)
-f.write(new_content)
-if self.TraceOn():
-print("new content:", new_content)
-print(
-"os.path.getmtime() after writing new content:",
-os.path.getmtime(self.file),
-)
+# clear the source cache.
+self.runCmd("source cache clear")
 
-  

[Lldb-commits] [PATCH] D153834: [lldb] Add two-level caching in the source manager

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Core/SourceManager.cpp:86
 
-  // It the target source path map has been updated, get this file again so we
-  // can successfully remap the source file
-  if (target_sp && file_sp &&
-  file_sp->GetSourceMapModificationID() !=
-  target_sp->GetSourcePathMap().GetModificationID())
-file_sp.reset();
+  if (!debugger_sp && debugger_sp->GetUseSourceCache()) {
+LLDB_LOG(log, "Source file caching disabled: creating new source file: 
{0}",

bulbazord wrote:
> This should be `if (debugger_sp` right? If `debugger_sp` is `false` (because 
> it's nullptr) then you'll end up running `debugger_sp->GetUseSourceCache()` 
> and die.
Yes, you're right. This should be `if(!debugger_sp || 
!debugger_sp->GetUseSourceCache())`


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

https://reviews.llvm.org/D153834

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

[Lldb-commits] [PATCH] D153834: [lldb] Add two-level caching in the source manager

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested changes to this revision.
bulbazord added a comment.
This revision now requires changes to proceed.

LGTM with a few nits. There is one piece of logic that I think does need to be 
changed before this can go in however.




Comment at: lldb/source/Commands/CommandObjectSource.cpp:1223-1224
+// Dump the process source cache.
+ProcessSP process_sp = m_exe_ctx.GetProcessSP();
+if (process_sp) {
+  result.GetOutputStream() << "\nProcess Source File Cache\n";

nit:



Comment at: lldb/source/Commands/CommandObjectSource.cpp:1250-1251
+// Clear the process cache.
+ProcessSP process_sp = m_exe_ctx.GetProcessSP();
+if (process_sp)
+  process_sp->GetSourceFileCache().Clear();

Same here



Comment at: lldb/source/Core/SourceManager.cpp:86
 
-  // It the target source path map has been updated, get this file again so we
-  // can successfully remap the source file
-  if (target_sp && file_sp &&
-  file_sp->GetSourceMapModificationID() !=
-  target_sp->GetSourcePathMap().GetModificationID())
-file_sp.reset();
+  if (!debugger_sp && debugger_sp->GetUseSourceCache()) {
+LLDB_LOG(log, "Source file caching disabled: creating new source file: 
{0}",

This should be `if (debugger_sp` right? If `debugger_sp` is `false` (because 
it's nullptr) then you'll end up running `debugger_sp->GetUseSourceCache()` and 
die.


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

https://reviews.llvm.org/D153834

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

[Lldb-commits] [PATCH] D153834: [lldb] Add two-level caching in the source manager

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: bulbazord, clayborg, jingham, jasonmolenda.
Herald added a project: All.
JDevlieghere requested review of this revision.

We recently saw an uptick in internal reports complaining that LLDB is slow 
when source on NFS are inaccessible. I looked at the SourceManger and its cache 
and I think there’s some room for improvement in terms of reducing file system 
accesses:

1. We always resolve the path.
2. We always check the timestamp.
3. We always recheck the file system for negative cache hits.

D153726  fixes (1) but (2) and (3) are 
necessary because of the cache’s current design. Source files are cached at the 
debugger level which means that the source file cache can span multiple targets 
and processes. It wouldn't be correct to not reload a modified or new file from 
disk.

We can however significantly reduce the number of file system accesses by using 
a two level cache design: one cache at the debugger level and one at the 
process level.

- The cache at the debugger level works the way it does today. There is no 
negative cache: if we can't find the file on disk, we'll try again next time 
the cache is queried. If a cached file's timestamp changes or if its path 
remapping changes, the cached file is evicted and we reload it from disk.
- The cache at the process level is design to avoid accessing the file system. 
It doesn't check the file's modification time. It caches negative results, so 
if a file didn't exist, it doesn't try to reread it from disk. Checking if the 
path remapping changed is cheap (doesn't involve checking the file system) so 
this is the only way for a file to get evicted from the process cache.

The result of this patch is that LLDB will not show you new content if a file 
is modified or created while a process is running. I would argue that this is 
what most people would expect, but it is a change from how LLDB behaves today.

For an average stop, we query the cache four times. With the current 
implementation, that's 4 stats to get the modification time, If the file 
doesn't exist on disk, that's another 4 stats. Before D153726 
, if the path starts with a `~` there are 
another additional 4 calls to realpath. When debugging sources on a slow 
(network) filesystem, this quickly adds up.

In addition to the two level caching, this patch also adds a `source` logging 
channel and synchronization to the source file cache. The logging was helpful 
during development and hopefully will help us triage issues in the future. The 
synchronization isn't a new requirement: as the cache is shared across targets, 
there is no guarantees that it can't be accessed concurrently.


https://reviews.llvm.org/D153834

Files:
  lldb/include/lldb/Core/SourceManager.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Utility/LLDBLog.h
  lldb/source/Commands/CommandObjectSource.cpp
  lldb/source/Core/SourceManager.cpp
  lldb/source/Utility/LLDBLog.cpp
  lldb/test/API/source-manager/TestSourceManager.py
  lldb/unittests/Core/SourceManagerTest.cpp

Index: lldb/unittests/Core/SourceManagerTest.cpp
===
--- lldb/unittests/Core/SourceManagerTest.cpp
+++ lldb/unittests/Core/SourceManagerTest.cpp
@@ -30,7 +30,7 @@
   // Insert: foo
   FileSpec foo_file_spec("foo");
   auto foo_file_sp =
-  std::make_shared(foo_file_spec, nullptr);
+  std::make_shared(foo_file_spec, lldb::DebuggerSP());
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
 
   // Query: foo, expect found.
@@ -44,7 +44,7 @@
   // Insert: foo
   FileSpec foo_file_spec("foo");
   auto foo_file_sp =
-  std::make_shared(foo_file_spec, nullptr);
+  std::make_shared(foo_file_spec, lldb::DebuggerSP());
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
 
   // Query: bar, expect not found.
@@ -62,8 +62,8 @@
   FileSystem::Instance().Resolve(resolved_foo_file_spec);
 
   // Create the file with the resolved file spec.
-  auto foo_file_sp =
-  std::make_shared(resolved_foo_file_spec, nullptr);
+  auto foo_file_sp = std::make_shared(
+  resolved_foo_file_spec, lldb::DebuggerSP());
 
   // Cache the result with the unresolved file spec.
   cache.AddSourceFile(foo_file_spec, foo_file_sp);
Index: lldb/test/API/source-manager/TestSourceManager.py
===
--- lldb/test/API/source-manager/TestSourceManager.py
+++ lldb/test/API/source-manager/TestSourceManager.py
@@ -10,6 +10,7 @@
 """
 
 import os
+import io
 import stat
 
 import lldb
@@ -37,6 +38,33 @@
 self.file = self.getBuildArtifact("main-copy.c")
 self.line = line_number("main.c", "// Set break point at this line.")
 
+def modify_content(self):
+
+# Read the main.c file content.
+with io.open(self.file, "r", newline="\n") as f:
+original_content = f.read()
+