Avoid expression evaluation in libStdC++ std::vector<bool> synthetic children provider #108414

jeffreytan81 · 2024-09-12T16:14:43Z

Our customers is reporting a serious performance issue (expanding a this pointer takes 70 seconds in VSCode) in a specific execution context.

Profiling shows the hot path is triggered by an expression evaluation from libStdC++ synthetic children provider for std::vector<bool> since it uses CreateValueFromExpression().

This PR added a new SBValue::CreateBoolValue() API and switch std::vector<bool> synthetic children provider to use the new API without performing expression evaluation.

Note: there might be other cases of CreateValueFromExpression() in our summary/synthetic children providers which I will sweep through in later PRs.

With this PR, the customer's scenario reduces from 70 seconds => 50 seconds. I will add other PRs to further optimize the remaining 50 seconds (mostly from type/namespace lookup).

Testing:
test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/vbool/TestDataFormatterStdVBool.py passes with the PR

llvmbot · 2024-09-12T16:15:15Z

@llvm/pr-subscribers-lldb

Author: None (jeffreytan81)

Changes

Our customers is reporting a serious performance issue (expanding a this pointer takes 70 seconds in VSCode) in a specific execution context.

Profiling shows the hot path is triggered by an expression evaluation from libStdC++ synthetic children provider for std::vector<bool> since it uses CreateValueFromExpression().

This PR added a new SBValue::CreateBoolValue() API and switch std::vector<bool> synthetic children provider to use the new API without performing expression evaluation.

Note: there might be other cases of CreateValueFromExpression() in our summary/synthetic children providers which I will sweep through in later PRs.

With this PR, the customer's scenario reduces from 70 seconds => 50 seconds. I will add other PRs to further optimize the remaining 50 seconds (mostly from type/namespace lookup).

Full diff: https://github.com/llvm/llvm-project/pull/108414.diff

3 Files Affected:

(modified) lldb/examples/synthetic/gnu_libstdcpp.py (+1-5)
(modified) lldb/include/lldb/API/SBValue.h (+2)
(modified) lldb/source/API/SBValue.cpp (+27)

diff --git a/lldb/examples/synthetic/gnu_libstdcpp.py b/lldb/examples/synthetic/gnu_libstdcpp.py
index d98495b8a9df38..597298dfce36b4 100644
--- a/lldb/examples/synthetic/gnu_libstdcpp.py
+++ b/lldb/examples/synthetic/gnu_libstdcpp.py
@@ -473,11 +473,7 @@ def get_child_at_index(self, index):
                 "[" + str(index) + "]", element_offset, element_type
             )
             bit = element.GetValueAsUnsigned(0) & (1 << bit_offset)
-            if bit != 0:
-                value_expr = "(bool)true"
-            else:
-                value_expr = "(bool)false"
-            return self.valobj.CreateValueFromExpression("[%d]" % index, value_expr)
+            return self.valobj.CreateBooleanValue("[%d]" % index, bool(bit))
 
         def update(self):
             try:
diff --git a/lldb/include/lldb/API/SBValue.h b/lldb/include/lldb/API/SBValue.h
index bec816fb451844..aeda26cb6dd795 100644
--- a/lldb/include/lldb/API/SBValue.h
+++ b/lldb/include/lldb/API/SBValue.h
@@ -146,6 +146,8 @@ class LLDB_API SBValue {
   lldb::SBValue CreateValueFromData(const char *name, lldb::SBData data,
                                     lldb::SBType type);
 
+  lldb::SBValue CreateBoolValue(const char *name, bool value);
+
   /// Get a child value by index from a value.
   ///
   /// Structs, unions, classes, arrays and pointers have child
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index 273aac5ad47989..eb54213f4e60bc 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -645,6 +645,33 @@ lldb::SBValue SBValue::CreateValueFromData(const char *name, SBData data,
   return sb_value;
 }
 
+lldb::SBValue SBValue::CreateBoolValue(const char *name, bool value) {
+  LLDB_INSTRUMENT_VA(this, name);
+
+  lldb::SBValue sb_value;
+  lldb::ValueObjectSP new_value_sp;
+  ValueLocker locker;
+  lldb::ValueObjectSP value_sp(GetSP(locker));
+  ProcessSP process_sp = m_opaque_sp->GetProcessSP();
+  lldb::SBTarget target = GetTarget();
+  if (!target.IsValid())
+    return sb_value;
+  lldb::SBType boolean_type = target.GetBasicType(lldb::eBasicTypeBool);
+  lldb::TypeImplSP type_impl_sp(boolean_type.GetSP());
+  if (value_sp && process_sp && type_impl_sp) {
+    int data_buf[1] = {value ? 1 : 0};
+    lldb::SBData data = lldb::SBData::CreateDataFromSInt32Array(
+        process_sp->GetByteOrder(), sizeof(data_buf[0]), data_buf,
+        sizeof(data_buf) / sizeof(data_buf[0]));
+    ExecutionContext exe_ctx(value_sp->GetExecutionContextRef());
+    new_value_sp = ValueObject::CreateValueObjectFromData(
+        name, **data, exe_ctx, type_impl_sp->GetCompilerType(true));
+    new_value_sp->SetAddressTypeOfChildren(eAddressTypeLoad);
+  }
+  sb_value.SetSP(new_value_sp);
+  return sb_value;
+}
+
 SBValue SBValue::GetChildAtIndex(uint32_t idx) {
   LLDB_INSTRUMENT_VA(this, idx);

Jlalond · 2024-09-12T16:35:22Z

lldb/source/API/SBValue.cpp

+  lldb::SBType boolean_type = target.GetBasicType(lldb::eBasicTypeBool);
+  lldb::TypeImplSP type_impl_sp(boolean_type.GetSP());
+  if (value_sp && process_sp && type_impl_sp) {
+    int data_buf[1] = {value ? 1 : 0};


Is a data buffer the only option here because of the bitwise logic? I ask because we could just have the address point to a singular byte instead right?

Jlalond · 2024-09-12T16:36:48Z

lldb/include/lldb/API/SBValue.h

@@ -146,6 +146,8 @@ class LLDB_API SBValue {
  lldb::SBValue CreateValueFromData(const char *name, lldb::SBData data,
                                    lldb::SBType type);

+  lldb::SBValue CreateBoolValue(const char *name, bool value);


I would add a blurb this is created from data not address

jimingham · 2024-09-12T18:01:54Z

The goal here is excellent! Not only is this a performance problem but if you call the version of GetValueFromExpression that doesn't take an expression options - as the code you are replacing does - you get the default value for "TryAllThreads" - which is true. So if you are unlucky and the expression takes a little bit of time (maybe your machine is heavily loaded or something) then printing this summary could cause other threads to make progress out from under you. In a GUI, that can result in the not helpful behavior where you stop at some point in Thread A, switch to Thread B - at which point the GUI loads the variables and runs the formatter for you and in doing so Thread A gets a chance to run. Then you switch back to Thread A and it wasn't where you left it. Bad debugger!

We should not use expressions in data formatters because they make them slow as you have seen. But we for sure should never call an expression with TryAllThreads set to true in a data formatter. If you are going to use expressions at all you should do it on only one thread and make sure that along the path of the function you call nobody takes locks that might deadlock the expression against other threads... But we should try hard not to make that necessary, and shouldn't do so in ones we ship.

Another general rule is that if you are adding a significant algorithm to an SB API implementation, you should really make an API in the underlying lldb_private class and then call that. We used to have all sorts of SB API specific implementations, but then somebody would need a similar thing on the lldb_private side, wouldn't notice the SB implementation, and duplicate it. That leads to code duplication and subtle differences in behavior depending on whether you find your way to the SB or private implementation. We try not to do that anymore.

Can you do this instead with a ValueObject::CreateBooleanValue, and then just wrap that in the SBValue::CreateBooleanValue call?

jimingham

This is good except the implementation should be in ValueObject not SBValue.

jeffreytan81 · 2024-09-12T21:40:31Z

@jimingham, sounds good. I will move the logic into ValueObject.

TryAllThreads is true

I did not see any client code setting this to true, I assume this happens because m_try_others is true by default? Maybe we should set m_try_others's default value to false to avoid side effect like this?

It sucks that we have to use timeout to guess and detect potential deadlock and resume all threads. On Windows, I remember I implemented the deadlock-free expression evaluation for VS debugger much accurately because Windows has an API to detect mutex block chain. With the APIs, I can even find out which dependency thread to resume to resolve the deadlock. It does not cover all synchronization primitives though.

jeffreytan81 · 2024-09-12T21:43:26Z

Also, I want to point out that, in this specific example, IRInterpreter will try to interpret the "(bool)true" expression so it won't run the thread plan with TryAllThreads = true, but I assume you are commenting the danger in a general sense.

jimingham · 2024-09-12T21:54:19Z

You are right, TryAllThreads defaults to True.

Not using TryAllThreads is actually dangerous in the general case. For instance, you can have the thread you run take a lock, then call some code that tries another lock that is held by a thread we aren't running. That times out so we cancel the execution evaluation but since we don't in general know about all the locking strategies available on any given platform, we don't know to unlock the first lock, and that will cause the program to deadlock later on if anybody tries to get the first lock again.

If you do the same thing with TryAllThreads set to true, then when the expression deadlocks on the running thread, we give a chance for the thread holding the second lock to release it, and the evaluation goes on to succeed, releasing the first lock on the way out.

Also, a very common workflow in debugging is to stop somewhere at a breakpoint, run a couple of expressions and continue, and you don't really care about the state of other threads. So true is the right default value.

There is an overload of CreateValueFromExpression that takes an EvaluteExpressionOptions, and if you are calling code internally you should be using that one because you really should think about how you want the expression to run.

jeffreytan81 · 2024-09-12T21:58:06Z

It's funny that there is already a ValueObject::CreateValueObjectFromBool() function :-)

jimingham · 2024-09-12T22:01:46Z

BTW, I agree that the "run one thread, then run all threads" strategy is unfortunate. But I can't think of a better way to do that that's likely to be supported on all platforms. It might be good at some point to add a "Lock detection plugin" that could provide enough information to be able to work our way past locking issues without requiring this strategy. That's bound to be system specific, so it needs to be a plugin. But I'm not sanguine that would be implementable on all platforms, so we're likely stuck with the current strategy.

The next stage in your "run single thread with timeout" feature would be to remove RunThreadPlan from Process and replace that with your new machinery. That will at least make the code shared and easier to work on.

jeffreytan81 · 2024-09-12T22:03:38Z

You are right, TryAllThreads defaults to True.

Not using TryAllThreads is actually dangerous in the general case. For instance, you can have the thread you run take a lock, then call some code that tries another lock that is held by a thread we aren't running. That times out so we cancel the execution evaluation but since we don't in general know about all the locking strategies available on any given platform, we don't know to unlock the first lock, and that will cause the program to deadlock later on if anybody tries to get the first lock again.

If you do the same thing with TryAllThreads set to true, then when the expression deadlocks on the running thread, we give a chance for the thread holding the second lock to release it, and the evaluation goes on to succeed, releasing the first lock on the way out.

Also, a very common workflow in debugging is to stop somewhere at a breakpoint, run a couple of expressions and continue, and you don't really care about the state of other threads. So true is the right default value.

There is an overload of CreateValueFromExpression that takes an EvaluteExpressionOptions, and if you are calling code internally you should be using that one because you really should think about how you want the expression to run.

Yeah, I see your point. VS debugger takes the other design decision to not run all threads, so it is notorious for expression evaluation causing IDE to deadlock or hang by accident during stepping. Eventually, they designed a UI to not automatically evaluate expressions in watch window during stop but has a "dangerous" button next to each expression saying click at your own risk.

jimingham · 2024-09-12T22:04:45Z

It's funny that there is already a ValueObject::CreateValueObjectFromBool() function :-)

The very thing I was worried about!

Might be a good idea to put the general "don't do substantial implementations in SB API" principle in the "SBI API" doc page. I did a quick scan and didn't see anything that stated that.

jimingham · 2024-09-12T22:08:35Z

You are right, TryAllThreads defaults to True.
Not using TryAllThreads is actually dangerous in the general case. For instance, you can have the thread you run take a lock, then call some code that tries another lock that is held by a thread we aren't running. That times out so we cancel the execution evaluation but since we don't in general know about all the locking strategies available on any given platform, we don't know to unlock the first lock, and that will cause the program to deadlock later on if anybody tries to get the first lock again.
If you do the same thing with TryAllThreads set to true, then when the expression deadlocks on the running thread, we give a chance for the thread holding the second lock to release it, and the evaluation goes on to succeed, releasing the first lock on the way out.
Also, a very common workflow in debugging is to stop somewhere at a breakpoint, run a couple of expressions and continue, and you don't really care about the state of other threads. So true is the right default value.
There is an overload of CreateValueFromExpression that takes an EvaluteExpressionOptions, and if you are calling code internally you should be using that one because you really should think about how you want the expression to run.

Yeah, I see your point. VS debugger takes the other design decision to not run all threads, so it is notorious for expression evaluation causing IDE to deadlock or hang by accident during stepping. Eventually, they designed a UI to not automatically evaluate expressions in watch window during stop but has a "dangerous" button next to each expression saying click at your own risk.

The earliest dogfooding team for lldb - way back before it was public - was the clang team not surprisingly. clang developers live by dump() so having a robust expression evaluator was one of the very early requirements! BTW, in general I don't suggest having one of the hardest features to implement in your project be the first hard requirement...

jimingham · 2024-09-12T22:11:28Z

lldb/source/API/SBValue.cpp

+  if (value_sp && target_sp) {
+    new_value_sp =
+        ValueObject::CreateValueObjectFromBool(target_sp, value, name);
+    new_value_sp->SetAddressTypeOfChildren(eAddressTypeLoad);


Did the lack of this line cause problems? bool values can't have children, so it's a bit surprising you had to specify this.

I copied it from SBValue::CreateValueFromData(). I actually do not accurately know its meaning, so simply keep it here for safety purpose. I will remove it since test still succeeds after removing.

The problem comes with ValueObjects that live in lldb host memory for instance ValueObjectConstResult "history" entities or ValueObject made from DataExtractors. If you have such a result value that includes pointers that you copied up from the target memory, even though the ValueObject itself lives in lldb host memory, the pointer children still point into the target (are load addresses). This API states that that is true for this ValueObject.

It's not relevant for ValueObjects that can't have pointer children.

jimingham · 2024-09-12T22:28:34Z

It's funny that there is already a ValueObject::CreateValueObjectFromBool() function :-)

The very thing I was worried about!

Might be a good idea to put the general "don't do substantial implementations in SB API" principle in the "SBI API" doc page. I did a quick scan and didn't see anything that stated that.

I put up a PR for that:

#108462

jimingham

LGTM

jeffreytan81 and others added 5 commits August 19, 2024 10:57

Fix StartDebuggingRequestHandler/ReplModeRequestHandler in lldb-dap

6e84ab9

Merge branch 'llvm:main' into main

b23f3ea

Merge branch 'llvm:main' into main

b8ec0fb

Merge branch 'llvm:main' into main

1a350e4

Add CreateBoolValue API

d56c9b3

jeffreytan81 requested review from clayborg, kusmour, zhyty and Jlalond September 12, 2024 16:14

jeffreytan81 requested a review from JDevlieghere as a code owner September 12, 2024 16:14

llvmbot added the lldb label Sep 12, 2024

Jlalond reviewed Sep 12, 2024

View reviewed changes

Jlalond approved these changes Sep 12, 2024

View reviewed changes

Fix typo

37a5ffa

jimingham self-requested a review September 12, 2024 18:03

jimingham requested changes Sep 12, 2024

View reviewed changes

Use ValueObject::CreateValueObjectFromBool() API

257cd0d

jimingham reviewed Sep 12, 2024

View reviewed changes

Remove SetAddressTypeOfChildren() call

713242a

jimingham approved these changes Sep 13, 2024

View reviewed changes

jeffreytan81 merged commit b6bf27e into llvm:main Sep 13, 2024
7 checks passed

jeffreytan81 mentioned this pull request Sep 17, 2024

Improve type and namespace lookup using parent chain #108907

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid expression evaluation in libStdC++ std::vector<bool> synthetic children provider #108414

Avoid expression evaluation in libStdC++ std::vector<bool> synthetic children provider #108414

jeffreytan81 commented Sep 12, 2024 •

edited

Loading

llvmbot commented Sep 12, 2024

Jlalond Sep 12, 2024

Jlalond Sep 12, 2024

jimingham commented Sep 12, 2024 •

edited

Loading

jimingham left a comment

jeffreytan81 commented Sep 12, 2024 •

edited

Loading

jeffreytan81 commented Sep 12, 2024

jimingham commented Sep 12, 2024

jeffreytan81 commented Sep 12, 2024 •

edited

Loading

jimingham commented Sep 12, 2024

jeffreytan81 commented Sep 12, 2024 •

edited

Loading

jimingham commented Sep 12, 2024

jimingham commented Sep 12, 2024

jimingham Sep 12, 2024

jeffreytan81 Sep 13, 2024

jimingham Sep 13, 2024

jimingham commented Sep 12, 2024

jimingham left a comment

Avoid expression evaluation in libStdC++ std::vector<bool> synthetic children provider #108414

Avoid expression evaluation in libStdC++ std::vector<bool> synthetic children provider #108414

Conversation

jeffreytan81 commented Sep 12, 2024 • edited Loading

llvmbot commented Sep 12, 2024

Jlalond Sep 12, 2024

Choose a reason for hiding this comment

Jlalond Sep 12, 2024

Choose a reason for hiding this comment

jimingham commented Sep 12, 2024 • edited Loading

jimingham left a comment

Choose a reason for hiding this comment

jeffreytan81 commented Sep 12, 2024 • edited Loading

jeffreytan81 commented Sep 12, 2024

jimingham commented Sep 12, 2024

jeffreytan81 commented Sep 12, 2024 • edited Loading

jimingham commented Sep 12, 2024

jeffreytan81 commented Sep 12, 2024 • edited Loading

jimingham commented Sep 12, 2024

jimingham commented Sep 12, 2024

jimingham Sep 12, 2024

Choose a reason for hiding this comment

jeffreytan81 Sep 13, 2024

Choose a reason for hiding this comment

jimingham Sep 13, 2024

Choose a reason for hiding this comment

jimingham commented Sep 12, 2024

jimingham left a comment

Choose a reason for hiding this comment

jeffreytan81 commented Sep 12, 2024 •

edited

Loading

jimingham commented Sep 12, 2024 •

edited

Loading

jeffreytan81 commented Sep 12, 2024 •

edited

Loading

jeffreytan81 commented Sep 12, 2024 •

edited

Loading

jeffreytan81 commented Sep 12, 2024 •

edited

Loading