Lazy deference underlying object for shared/weak/unique_ptr synthetic… #67069
Conversation
… provider We noticed some performance issue while in lldb-vscode for grabing the name of the SBValue. Profiling shows SBValue::GetName() can cause synthetic children provider of shared/unique_ptr to deference underlying object and complete it type. This patch lazily moves the dereference from synthetic child provider's Update() method to GetChildAtIndex() so that SBValue::GetName() won't trigger the slow code path. Here is the culprit slow code path: ``` ... frame llvm#59: 0x00007ff4102e0660 liblldb.so.15`SymbolFileDWARF::CompleteType(this=<unavailable>, compiler_type=0x00007ffdd9829450) at SymbolFileDWARF.cpp:1567:25 [opt] ... frame llvm#67: 0x00007ff40fdf9bd4 liblldb.so.15`lldb_private::ValueObject::Dereference(this=0x0000022bb5dfe980, error=0x00007ffdd9829970) at ValueObject.cpp:2672:41 [opt] frame llvm#68: 0x00007ff41011bb0a liblldb.so.15`(anonymous namespace)::LibStdcppSharedPtrSyntheticFrontEnd::Update(this=0x000002298fb94380) at LibStdcpp.cpp:403:40 [opt] frame llvm#69: 0x00007ff41011af9a liblldb.so.15`lldb_private::formatters::LibStdcppSharedPtrSyntheticFrontEndCreator(lldb_private::CXXSyntheticChildren*, std::shared_ptr<lldb_private::ValueObject>) [inlined] (anonymous namespace)::LibStdcppSharedPtrSyntheticFrontEnd::LibStdcppSharedPtrSyntheticFrontEnd(this=0x000002298fb94380, valobj_sp=<unavailable>) at LibStdcpp.cpp:371:5 [opt] ... frame llvm#78: 0x00007ff40fdf6e42 liblldb.so.15`lldb_private::ValueObject::CalculateSyntheticValue(this=0x000002296c66a500) at ValueObject.cpp:1836:27 [opt] frame llvm#79: 0x00007ff40fdf1939 liblldb.so.15`lldb_private::ValueObject::GetSyntheticValue(this=<unavailable>) at ValueObject.cpp:1867:3 [opt] frame llvm#80: 0x00007ff40fc89008 liblldb.so.15`ValueImpl::GetSP(this=0x0000022c71b90de0, stop_locker=0x00007ffdd9829d00, lock=0x00007ffdd9829d08, error=0x00007ffdd9829d18) at SBValue.cpp:141:46 [opt] frame llvm#81: 0x00007ff40fc7d82a liblldb.so.15`lldb::SBValue::GetSP(ValueLocker&) const [inlined] ValueLocker::GetLockedSP(this=0x00007ffdd9829d00, in_value=<unavailable>) at SBValue.cpp:208:21 [opt] frame llvm#82: 0x00007ff40fc7d817 liblldb.so.15`lldb::SBValue::GetSP(this=0x00007ffdd9829d90, locker=0x00007ffdd9829d00) const at SBValue.cpp:1047:17 [opt] frame llvm#83: 0x00007ff40fc7da6f liblldb.so.15`lldb::SBValue::GetName(this=0x00007ffdd9829d90) at SBValue.cpp:294:32 [opt] ... ``` Differential Revision: https://reviews.llvm.org/D159542
jeffreytan81
requested review from
clayborg,
zhyty,
Michael137 and
GeorgeHuyubo
|
@llvm/pr-subscribers-lldb ChangesWe noticed some performance issue while in lldb-vscode for grabing the name of the SBValue. Profiling shows SBValue::GetName() can cause synthetic children provider of shared/unique_ptr to deference underlying object and complete it type. This patch lazily moves the dereference from synthetic child provider's Update() method to GetChildAtIndex() so that SBValue::GetName() won't trigger the slow code path. Here is the culprit slow code path: Differential Revision: https://reviews.llvm.org/D159542 Full diff: https://github.com/llvm/llvm-project/pull/67069.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp
index c65bb9b6bc9b187..23af50fdb7124ec 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp
@@ -377,9 +377,16 @@ lldb::ValueObjectSP
LibStdcppSharedPtrSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
if (idx == 0)
return m_ptr_obj->GetSP();
- if (idx == 1)
- return m_obj_obj->GetSP();
-
+ if (idx == 1) {
+ if (m_ptr_obj && !m_obj_obj) {
+ Status error;
+ ValueObjectSP obj_obj = m_ptr_obj->Dereference(error);
+ if (error.Success())
+ m_obj_obj = obj_obj->Clone(ConstString("object")).get();
+ }
+ if (m_obj_obj)
+ return m_obj_obj->GetSP();
+ }
return lldb::ValueObjectSP();
}
@@ -397,14 +404,7 @@ bool LibStdcppSharedPtrSyntheticFrontEnd::Update() {
return false;
m_ptr_obj = ptr_obj_sp->Clone(ConstString("pointer")).get();
-
- if (m_ptr_obj) {
- Status error;
- ValueObjectSP obj_obj = m_ptr_obj->Dereference(error);
- if (error.Success()) {
- m_obj_obj = obj_obj->Clone(ConstString("object")).get();
- }
- }
+ m_obj_obj = nullptr;
return false;
}
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
index 7174e9102e1bd0a..a84d641b57bc475 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
@@ -108,14 +108,7 @@ bool LibStdcppUniquePtrSyntheticFrontEnd::Update() {
if (del_obj)
m_del_obj = del_obj->Clone(ConstString("deleter")).get();
}
-
- if (m_ptr_obj) {
- Status error;
- ValueObjectSP obj_obj = m_ptr_obj->Dereference(error);
- if (error.Success()) {
- m_obj_obj = obj_obj->Clone(ConstString("object")).get();
- }
- }
+ m_obj_obj = nullptr;
return false;
}
@@ -128,8 +121,17 @@ LibStdcppUniquePtrSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
return m_ptr_obj->GetSP();
if (idx == 1 && m_del_obj)
return m_del_obj->GetSP();
- if (idx == 2 && m_obj_obj)
- return m_obj_obj->GetSP();
+ if (idx == 2) {
+ if (m_ptr_obj && !m_obj_obj) {
+ Status error;
+ ValueObjectSP obj_obj = m_ptr_obj->Dereference(error);
+ if (error.Success()) {
+ m_obj_obj = obj_obj->Clone(ConstString("object")).get();
+ }
+ }
+ if (m_obj_obj)
+ return m_obj_obj->GetSP();
+ }
return lldb::ValueObjectSP();
}
|
|
Supposedly, Update returns true or false based on whether the update succeeds. If we took that seriously, then it would be worrisome that this patch has moved one of the possible failure modes from Update to GetChildAtIndex - losing the chance to trap the error earlier. But both these Update functions return false no matter what happens, so I guess we must not take that return value seriously? If we don't, we should probably stop returning it? |
|
@jimingham , IIRC from old experiments regarding the synthetic formatter code, |
|
How right you are. I was thinking this was the ValueObject updated stuff, but it's the SyntheticChildrenFrontEnd::Update. If I'm reading the code in ValueObjectSyntheticFilter.cpp aright, the return for that actually means the opposite of what you remembered. If you return So for instance, in the case where a synthetic child front end just hands out a ValueObjectChild in the tree of the underlying ValueObject, then the child that gets cached in the VOSynthetic::m_synthetic_children_cache knows how to update itself from its real parent, it doesn't need help from the ValueObjectSynthetic's FrontEndProvider to get the correct value. So there's no point removing it from the cache and making a new child every time we stop, the one in the cache knows how to keep its value up to date. But in the case where the children are, for instance, ValueObjectConstResults calculated by some means by the FrontEnd, to return I am pretty sure the "Dereferenced" child of a ValueObject can update itself from the VO that generated it, in which case, the two formatters in this patch could have returned true and saved the work of recreating the child VO as the dereference. But I haven't tested that theory, and I doubt just making up a new VO is expensive enough to make that worth the effort. The only downside is if you keep throwing the children away at every stop, the synthetic children have no hope of reporting "IsChanged" correctly. Anyway, not relevant to this patch. I think returning false was wrong before, but that's just a small bit more work, so it's not worth worrying over. |
|
Do you think it would be a good idea to provide aliases for the return values of the Update method with better names that match the behavior you just discovered? I'm not a fan of true/false in this case, because it confuses synthetic type developers, but I'm just trying to think of ways to improve this without breaking any existing code. |
|
The comment in front of the `SyntheticChildProviderFrontEnd::Update` is actually pretty clear about what it does, the problem is that it isn't really obvious from the comment when discarding the cached children would be a good or bad idea. I'm not sure you could express that in a name, we really need a better doc.
Jim
… On Sep 22, 2023, at 6:46 AM, Walter Erquinigo ***@***.***> wrote:
Do you think it would be a good idea to provide aliases for the return values of the Update method with better names that match the behavior you just discovered? I'm not a fan of true/false in this case, because it confuses synthetic type developers, but I'm just trying to think of ways to improve this without breaking any existing code.
—
Reply to this email directly, view it on GitHub <#67069 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW7CPPBX5CHQWSJBZP3X3WJDLANCNFSM6AAAAAA5CHOB7M>.
You are receiving this because you were mentioned.
|
We noticed some performance issue while in lldb-vscode for grabing the name of the SBValue. Profiling shows SBValue::GetName() can cause synthetic children provider of shared/unique_ptr to deference underlying object and complete it type.
This patch lazily moves the dereference from synthetic child provider's Update() method to GetChildAtIndex() so that SBValue::GetName() won't trigger the slow code path.
Here is the culprit slow code path:
Differential Revision: https://reviews.llvm.org/D159542