[Lldb-commits] [PATCH] D32306: Remove lock from ConstString::GetLength
scott.smith created this revision.
ConstStrings are immutable, so there is no need to grab even a reader lock in
order to read the length field.
Repository:
rL LLVM
https://reviews.llvm.org/D32306
Files:
source/Utility/ConstString.cpp
Index: source/Utility/ConstString.cpp
===
--- source/Utility/ConstString.cpp
+++ source/Utility/ConstString.cpp
@@ -42,10 +42,10 @@
return *reinterpret_cast(ptr);
}
- size_t GetConstCStringLength(const char *ccstr) const {
+ static size_t GetConstCStringLength(const char *ccstr) {
if (ccstr != nullptr) {
- const uint8_t h = hash(llvm::StringRef(ccstr));
- llvm::sys::SmartScopedReader rlock(m_string_pools[h].m_mutex);
+ // Since the entry is read only, and we derive the entry entirely from
the
+ // pointer, we don't need the lock.
const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr);
return entry.getKey().size();
}
@@ -218,10 +218,8 @@
if (m_string == rhs.m_string)
return false;
- llvm::StringRef lhs_string_ref(m_string,
- StringPool().GetConstCStringLength(m_string));
- llvm::StringRef rhs_string_ref(
- rhs.m_string, StringPool().GetConstCStringLength(rhs.m_string));
+ llvm::StringRef lhs_string_ref(GetStringRef());
+ llvm::StringRef rhs_string_ref(rhs.GetStringRef());
// If both have valid C strings, then return the comparison
if (lhs_string_ref.data() && rhs_string_ref.data())
@@ -240,7 +238,7 @@
}
size_t ConstString::GetLength() const {
- return StringPool().GetConstCStringLength(m_string);
+ return Pool::GetConstCStringLength(m_string);
}
bool ConstString::Equals(const ConstString &lhs, const ConstString &rhs,
@@ -255,10 +253,8 @@
return false;
// perform case insensitive equality test
- llvm::StringRef lhs_string_ref(
- lhs.m_string, StringPool().GetConstCStringLength(lhs.m_string));
- llvm::StringRef rhs_string_ref(
- rhs.m_string, StringPool().GetConstCStringLength(rhs.m_string));
+ llvm::StringRef lhs_string_ref(lhs.GetStringRef());
+ llvm::StringRef rhs_string_ref(rhs.GetStringRef());
return lhs_string_ref.equals_lower(rhs_string_ref);
}
@@ -270,10 +266,8 @@
if (lhs_cstr == rhs_cstr)
return 0;
if (lhs_cstr && rhs_cstr) {
-llvm::StringRef lhs_string_ref(
-lhs_cstr, StringPool().GetConstCStringLength(lhs_cstr));
-llvm::StringRef rhs_string_ref(
-rhs_cstr, StringPool().GetConstCStringLength(rhs_cstr));
+llvm::StringRef lhs_string_ref(lhs.GetStringRef());
+llvm::StringRef rhs_string_ref(rhs.GetStringRef());
if (case_sensitive) {
return lhs_string_ref.compare(rhs_string_ref);
Index: source/Utility/ConstString.cpp
===
--- source/Utility/ConstString.cpp
+++ source/Utility/ConstString.cpp
@@ -42,10 +42,10 @@
return *reinterpret_cast(ptr);
}
- size_t GetConstCStringLength(const char *ccstr) const {
+ static size_t GetConstCStringLength(const char *ccstr) {
if (ccstr != nullptr) {
- const uint8_t h = hash(llvm::StringRef(ccstr));
- llvm::sys::SmartScopedReader rlock(m_string_pools[h].m_mutex);
+ // Since the entry is read only, and we derive the entry entirely from the
+ // pointer, we don't need the lock.
const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr);
return entry.getKey().size();
}
@@ -218,10 +218,8 @@
if (m_string == rhs.m_string)
return false;
- llvm::StringRef lhs_string_ref(m_string,
- StringPool().GetConstCStringLength(m_string));
- llvm::StringRef rhs_string_ref(
- rhs.m_string, StringPool().GetConstCStringLength(rhs.m_string));
+ llvm::StringRef lhs_string_ref(GetStringRef());
+ llvm::StringRef rhs_string_ref(rhs.GetStringRef());
// If both have valid C strings, then return the comparison
if (lhs_string_ref.data() && rhs_string_ref.data())
@@ -240,7 +238,7 @@
}
size_t ConstString::GetLength() const {
- return StringPool().GetConstCStringLength(m_string);
+ return Pool::GetConstCStringLength(m_string);
}
bool ConstString::Equals(const ConstString &lhs, const ConstString &rhs,
@@ -255,10 +253,8 @@
return false;
// perform case insensitive equality test
- llvm::StringRef lhs_string_ref(
- lhs.m_string, StringPool().GetConstCStringLength(lhs.m_string));
- llvm::StringRef rhs_string_ref(
- rhs.m_string, StringPool().GetConstCStringLength(rhs.m_string));
+ llvm::StringRef lhs_string_ref(lhs.GetStringRef());
+ llvm::StringRef rhs_string_ref(rhs.GetStringRef());
return lhs_string_ref.equals_lower(rhs_string_ref);
}
@@ -270,10 +266,8 @@
if (lhs_cstr == rhs_cstr)
return 0;
if (lhs_cstr && rhs_cstr) {
-llvm::StringRef lhs_string_ref(
-lhs_cstr, StringPool().GetConstCStringLength(lhs_cstr));
-llvm::StringRef rh
[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key
scott.smith created this revision.
UniqueCStringMap "sorts" the entries for fast lookup, but really it only cares
about uniqueness. ConstString can be compared by pointer along, rather than
with strcmp, resulting in much faster comparisons. Change the interface to
take ConstString instead, and propagate use of the type to the callers where
appropriate.
Repository:
rL LLVM
https://reviews.llvm.org/D32316
Files:
include/lldb/Core/UniqueCStringMap.h
include/lldb/Interpreter/Property.h
include/lldb/Symbol/ObjectFile.h
include/lldb/Utility/ConstString.h
source/Interpreter/OptionValueEnumeration.cpp
source/Interpreter/OptionValueProperties.cpp
source/Interpreter/Property.cpp
source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.h
source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
source/Plugins/SymbolFile/DWARF/NameToDIE.h
source/Symbol/ClangASTContext.cpp
source/Symbol/GoASTContext.cpp
source/Symbol/Symtab.cpp
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -263,16 +263,15 @@
continue;
const Mangled &mangled = symbol->GetMangled();
- entry.cstring = mangled.GetMangledName().GetStringRef();
- if (!entry.cstring.empty()) {
+ entry.cstring = mangled.GetMangledName();
+ if (entry.cstring) {
m_name_to_index.Append(entry);
if (symbol->ContainsLinkerAnnotations()) {
// If the symbol has linker annotations, also add the version without
// the annotations.
- entry.cstring = ConstString(m_objfile->StripLinkerSymbolAnnotations(
- entry.cstring))
- .GetStringRef();
+ entry.cstring = m_objfile->StripLinkerSymbolAnnotations(
+ entry.cstring);
m_name_to_index.Append(entry);
}
@@ -290,9 +289,8 @@
{
CPlusPlusLanguage::MethodName cxx_method(
mangled.GetDemangledName(lldb::eLanguageTypeC_plus_plus));
-entry.cstring =
-ConstString(cxx_method.GetBasename()).GetStringRef();
-if (!entry.cstring.empty()) {
+entry.cstring = ConstString(cxx_method.GetBasename());
+if (entry.cstring) {
// ConstString objects permanently store the string in the pool so
// calling
// GetCString() on the value gets us a const char * that will
@@ -341,17 +339,15 @@
}
}
- entry.cstring =
- mangled.GetDemangledName(symbol->GetLanguage()).GetStringRef();
- if (!entry.cstring.empty()) {
+ entry.cstring = mangled.GetDemangledName(symbol->GetLanguage());
+ if (entry.cstring) {
m_name_to_index.Append(entry);
if (symbol->ContainsLinkerAnnotations()) {
// If the symbol has linker annotations, also add the version without
// the annotations.
- entry.cstring = ConstString(m_objfile->StripLinkerSymbolAnnotations(
- entry.cstring))
- .GetStringRef();
+ entry.cstring = m_objfile->StripLinkerSymbolAnnotations(
+ entry.cstring);
m_name_to_index.Append(entry);
}
}
@@ -359,15 +355,15 @@
// If the demangled name turns out to be an ObjC name, and
// is a category name, add the version without categories to the index
// too.
- ObjCLanguage::MethodName objc_method(entry.cstring, true);
+ ObjCLanguage::MethodName objc_method(entry.cstring.GetStringRef(), true);
if (objc_method.IsValid(true)) {
-entry.cstring = objc_method.GetSelector().GetStringRef();
+entry.cstring = objc_method.GetSelector();
m_selector_to_index.Append(entry);
ConstString objc_method_no_category(
objc_method.GetFullNameWithoutCategory(true));
if (objc_method_no_category) {
- entry.cstring = objc_method_no_category.GetStringRef();
+ entry.cstring = objc_method_no_category;
m_name_to_index.Append(entry);
}
}
@@ -444,15 +440,14 @@
const Mangled &mangled = symbol->GetMangled();
if (add_demangled) {
-entry.cstring =
-mangled.GetDemangledName(symbol->GetLanguage()).GetStringRef();
-if (!entry.cstring.empty())
+entry.cstring = mangled.GetDemangledName(symbol->GetLanguage());
+if (entry.cstring)
name_to_index_map.Append(entry);
}
if (add_mangled) {
-entry.cstring = mangled.GetMangledName().GetStringRef(
[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key
scott.smith added a comment.
At a high level, I think there might be a misunderstanding on what I'm
attempting to do. It isn't to convert things that weren't ConstString into
things that are. It is instead to utilize the fact that we have all these
ConstString in order to improve the performance of various operations by
retaining the fact that they are ConstString. While a conversion from
ConstString to StringRef is relatively cheap (at least after a separate change
I have to remove the hashing of the string and the lock operation), the
conversion back to ConstString is expensive. If we pass around StringRef to
functions that need ConstString, then the performance will remain poor.
Comment at: include/lldb/Interpreter/Property.h:43
+ ConstString GetName() const { return m_name; }
+ ConstString GetDescription() const { return m_description; }
clayborg wrote:
> This shouldn't be const-ified, Only names of things like variables,
> enumerators, typenames, paths, and other strings that are going to be uniqued
> should go into the ConstStringPool
ok but note the original code - it's already storing m_name and m_description
as ConstString. All I'm doing is exposing the underlying type. Would you
prefer I change m_name and m_description to be std::string instead? Otherwise
it won't actually save anything.
Also note that later uses take the names are put into m_name_to_index in the
option parser, which is a UniqueCString, which requires ConstString. I don't
know the code well enough to know whether all options or only some options go
through this, so I could see it being worth it to only convert to ConstString
at that point (for example, see source/Interpreter/OptionValueProperties.cpp in
this review).
Comment at: include/lldb/Symbol/ObjectFile.h:808-811
+ virtual ConstString
+ StripLinkerSymbolAnnotations(ConstString symbol_name) const {
+return symbol_name;
}
clayborg wrote:
> This actually doesn't need to change. Since it is merely stripping off parts
> of the string, this should really stay as a StringRef and it should return a
> StringRef. Change to use StringRef for return and for argument, or revert.
>
The only user of StripLinkerSymbolAnnotations immediately converts it to a
ConstString. Changing this back to using StringRef would mean an extra lookup
for architectures that do not override this function.
Comment at: include/lldb/Utility/ConstString.h:481
+ char operator[](size_t Index) const {
+assert(Index < GetLength() && "Invalid index!");
+return m_string[Index];
clayborg wrote:
> I really don't like the prospect of crashing the debugger if someone calls
> this with an invalid index. Many people ship with asserts on. I would either
> make it safe if it is indeed just for reporting or remove the assert.
This code is copied from StringRef, which is what lldb used before this change.
Do you still want me to revert the assert?
Though IMO it should be changed to <=, not <, so that the null terminator can
be read safely.
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:324-326
+ ConstString key_cstr = m_impl.GetCStringAtIndex(item);
+ if (strstr(type_name.GetCString(), key_cstr.GetCString())) {
+count += AppendReplacements(type_name, key_cstr, equivalents);
clayborg wrote:
> StringRef is better to use for modifying simple strings and looking for
> things. Actually looking at this class it is using ConstString all over the
> place for putting strings together. For creating strings we should use
> lldb_private::StreamString. For stripping stuff off and grabbing just part of
> a string, seeing if a string starts with, ends with, contains, etc,
> llvm::StringRef. So I would vote to just change it back, or fix the entire
> class to use lldb_private::StreamString
I can revert this fragment, but just note this won't affect the number of
ConstStrings that are generated. m_impl has ConstStrings in it, and type_name
is a ConstString, so yes they can be converted to StringRef just to call
StringRef::contains, or we can leave them as ConstString and utilize strstr.
Or, I can add ConstString::contains so the interface to ConstString and
StringRef is more similar.
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:345-348
+ uint32_t AppendReplacements(ConstString original,
+ ConstString matching_key,
std::vector &equivalents) {
+std::string matching_key_str(matching_key.GetCString());
clayborg wrote:
> Ditto
In line 352 below, m_impl is a UniqueCStringMap, so all keys are ConstString.
The only way to look up in a UnqiueCStringMap is using ConstString, so
matching_key should remain a ConstString to prevent an unnecessary lookup.
Everything else is fo
[Lldb-commits] [PATCH] D32306: Remove lock from ConstString::GetLength
scott.smith added a comment. In https://reviews.llvm.org/D32306#733316, @labath wrote: > Looks good, thank you. > > Out of curiosity, have you observed any performance improvements resulting > from this? 10.2% reduction in # of instructions executed, 9.1% reduction in # of cycles, as measured by 'perf stat' in single threaded mode (I disabled TaskPool in order to get more repeatable results). Can you commit this for me? I don't have commit access. Thank you! Repository: rL LLVM https://reviews.llvm.org/D32306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32503: Remove unused code related to CPlusPlusLanguage::FindEquivalentNames
scott.smith created this revision.
It is simply unused, and the header for it is private, so there should be no
external dependencies.
Repository:
rL LLVM
https://reviews.llvm.org/D32503
Files:
source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
Index: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
===
--- source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
+++ source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
@@ -119,18 +119,6 @@
llvm::StringRef &context,
llvm::StringRef &identifier);
- // in some cases, compilers will output different names for one same type.
- // when that happens, it might be impossible
- // to construct SBType objects for a valid type, because the name that is
- // available is not the same as the name that
- // can be used as a search key in FindTypes(). the equivalents map here is
- // meant to return possible alternative names
- // for a type through which a search can be conducted. Currently, this is only
- // enabled for C++ but can be extended
- // to ObjC or other languages if necessary
- static uint32_t FindEquivalentNames(ConstString type_name,
- std::vector &equivalents);
-
// Given a mangled function name, calculates some alternative manglings since
// the compiler mangling may not line up with the symbol we are expecting
static uint32_t
Index: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -271,144 +271,6 @@
return false;
}
-class CPPRuntimeEquivalents {
-public:
- CPPRuntimeEquivalents() {
-m_impl.Append(ConstString("std::basic_string, "
- "std::allocator >")
- .GetStringRef(),
- ConstString("basic_string"));
-
-// these two (with a prefixed std::) occur when c++stdlib string class
-// occurs as a template argument in some STL container
-m_impl.Append(ConstString("std::basic_string, "
- "std::allocator >")
- .GetStringRef(),
- ConstString("std::basic_string"));
-
-m_impl.Sort();
- }
-
- void Add(ConstString &type_name, ConstString &type_equivalent) {
-m_impl.Insert(type_name.GetStringRef(), type_equivalent);
- }
-
- uint32_t FindExactMatches(ConstString &type_name,
-std::vector &equivalents) {
-uint32_t count = 0;
-
-for (ImplData match =
- m_impl.FindFirstValueForName(type_name.GetStringRef());
- match != nullptr; match = m_impl.FindNextValueForName(match)) {
- equivalents.push_back(match->value);
- count++;
-}
-
-return count;
- }
-
- // partial matches can occur when a name with equivalents is a template
- // argument.
- // e.g. we may have "class Foo" be a match for "struct Bar". if we have a
- // typename
- // such as "class Templatized" we want this to be
- // replaced with
- // "class Templatized". Since partial matching is time
- // consuming
- // once we get a partial match, we add it to the exact matches list for faster
- // retrieval
- uint32_t FindPartialMatches(ConstString &type_name,
- std::vector &equivalents) {
-uint32_t count = 0;
-
-llvm::StringRef type_name_cstr = type_name.GetStringRef();
-
-size_t items_count = m_impl.GetSize();
-
-for (size_t item = 0; item < items_count; item++) {
- llvm::StringRef key_cstr = m_impl.GetCStringAtIndex(item);
- if (type_name_cstr.contains(key_cstr)) {
-count += AppendReplacements(type_name_cstr, key_cstr, equivalents);
- }
-}
-
-return count;
- }
-
-private:
- std::string &replace(std::string &target, std::string &pattern,
- std::string &with) {
-size_t pos;
-size_t pattern_len = pattern.size();
-
-while ((pos = target.find(pattern)) != std::string::npos)
- target.replace(pos, pattern_len, with);
-
-return target;
- }
-
- uint32_t AppendReplacements(llvm::StringRef original,
- llvm::StringRef matching_key,
- std::vector &equivalents) {
-std::string matching_key_str(matching_key);
-ConstString original_const(original);
-
-uint32_t count = 0;
-
-for (ImplData match = m_impl.FindFirstValueForName(matching_key);
- match != nullptr; match = m_impl.FindNextValueForName(match)) {
- std::string target(original);
- std::string equiv_class(match->value.AsCString());
-
- replace(target, matching_key_str, equiv_class);
-
- ConstString target_const(target.c_str());
-
-// you wi
[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key
scott.smith updated this revision to Diff 96629.
scott.smith marked 21 inline comments as done.
scott.smith added a comment.
address review comments
Repository:
rL LLVM
https://reviews.llvm.org/D32316
Files:
include/lldb/Core/UniqueCStringMap.h
include/lldb/Symbol/ObjectFile.h
source/Interpreter/OptionValueEnumeration.cpp
source/Interpreter/OptionValueProperties.cpp
source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.h
source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
source/Plugins/SymbolFile/DWARF/NameToDIE.h
source/Symbol/ClangASTContext.cpp
source/Symbol/GoASTContext.cpp
source/Symbol/Symtab.cpp
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -263,36 +263,35 @@
continue;
const Mangled &mangled = symbol->GetMangled();
- entry.cstring = mangled.GetMangledName().GetStringRef();
- if (!entry.cstring.empty()) {
+ entry.cstring = mangled.GetMangledName();
+ if (entry.cstring) {
m_name_to_index.Append(entry);
if (symbol->ContainsLinkerAnnotations()) {
// If the symbol has linker annotations, also add the version without
// the annotations.
entry.cstring = ConstString(m_objfile->StripLinkerSymbolAnnotations(
- entry.cstring))
- .GetStringRef();
+entry.cstring.GetStringRef()));
m_name_to_index.Append(entry);
}
const SymbolType symbol_type = symbol->GetType();
if (symbol_type == eSymbolTypeCode ||
symbol_type == eSymbolTypeResolver) {
- if (entry.cstring[0] == '_' && entry.cstring[1] == 'Z' &&
- (entry.cstring[2] != 'T' && // avoid virtual table, VTT structure,
- // typeinfo structure, and typeinfo
- // name
- entry.cstring[2] != 'G' && // avoid guard variables
- entry.cstring[2] != 'Z')) // named local entities (if we
+ llvm::StringRef entry_ref(entry.cstring.GetStringRef());
+ if (entry_ref[0] == '_' && entry_ref[1] == 'Z' &&
+ (entry_ref[2] != 'T' && // avoid virtual table, VTT structure,
+ // typeinfo structure, and typeinfo
+ // name
+ entry_ref[2] != 'G' && // avoid guard variables
+ entry_ref[2] != 'Z')) // named local entities (if we
// eventually handle eSymbolTypeData,
// we will want this back)
{
CPlusPlusLanguage::MethodName cxx_method(
mangled.GetDemangledName(lldb::eLanguageTypeC_plus_plus));
-entry.cstring =
-ConstString(cxx_method.GetBasename()).GetStringRef();
-if (!entry.cstring.empty()) {
+entry.cstring = ConstString(cxx_method.GetBasename());
+if (entry.cstring) {
// ConstString objects permanently store the string in the pool so
// calling
// GetCString() on the value gets us a const char * that will
@@ -300,7 +299,8 @@
const char *const_context =
ConstString(cxx_method.GetContext()).GetCString();
- if (entry.cstring[0] == '~' ||
+ entry_ref = entry.cstring.GetStringRef();
+ if (entry_ref[0] == '~' ||
!cxx_method.GetQualifiers().empty()) {
// The first character of the demangled basename is '~' which
// means we have a class destructor. We can use this information
@@ -341,17 +341,15 @@
}
}
- entry.cstring =
- mangled.GetDemangledName(symbol->GetLanguage()).GetStringRef();
- if (!entry.cstring.empty()) {
+ entry.cstring = mangled.GetDemangledName(symbol->GetLanguage());
+ if (entry.cstring) {
m_name_to_index.Append(entry);
if (symbol->ContainsLinkerAnnotations()) {
// If the symbol has linker annotations, also add the version without
// the annotations.
entry.cstring = ConstString(m_objfile->StripLinkerSymbolAnnotations(
- entry.cstring))
- .GetStringRef();
+entry.cstring.GetStringRef()));
m_name_to_index.Append(entry);
}
}
@@ -359,15 +357,15 @@
// If the demangled name turns out to be an ObjC name, and
// is a category name, add the version without categories to the index
[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key
scott.smith added inline comments.
Comment at: include/lldb/Symbol/ObjectFile.h:808-811
+ virtual ConstString
+ StripLinkerSymbolAnnotations(ConstString symbol_name) const {
+return symbol_name;
}
scott.smith wrote:
> clayborg wrote:
> > This actually doesn't need to change. Since it is merely stripping off
> > parts of the string, this should really stay as a StringRef and it should
> > return a StringRef. Change to use StringRef for return and for argument, or
> > revert.
> >
> The only user of StripLinkerSymbolAnnotations immediately converts it to a
> ConstString. Changing this back to using StringRef would mean an extra
> lookup for architectures that do not override this function.
reverting this has no measurable performance impact on my test, so even though
the caller has a ConstString, and will convert the result to a ConstString, I
will revert this.
Comment at: include/lldb/Utility/ConstString.h:481
+ char operator[](size_t Index) const {
+assert(Index < GetLength() && "Invalid index!");
+return m_string[Index];
labath wrote:
> scott.smith wrote:
> > clayborg wrote:
> > > I really don't like the prospect of crashing the debugger if someone
> > > calls this with an invalid index. Many people ship with asserts on. I
> > > would either make it safe if it is indeed just for reporting or remove
> > > the assert.
> > This code is copied from StringRef, which is what lldb used before this
> > change. Do you still want me to revert the assert?
> >
> > Though IMO it should be changed to <=, not <, so that the null terminator
> > can be read safely.
> I think that asserting here is fine... There's no way you can make code that
> likes to tread off the end of an array safe by changing this function.
>
> However, I am not really fond of the idea of ConstString taking over
> StringRef functionality. I think we should stick to requiring stringref
> conversions when doing string manipulation. Maybe if you reduce the number of
> ConstString occurences according to other comments, this will not be that
> necessary anymore (?)
The indexing is used in Symtab::InitNameIndexes, where is dealing with
ConstString. IMO converting to a StringRef there is a bad idea, as it means
there will be two variables representing the same thing (one ConstString, one
StringRef), which makes it easy to update one and not the other.
But there is no performance difference, at least if review
https://reviews.llvm.org/D32306 is committed (which removes hashing and locking
when getting the length). Otherwise it's a >1% penalty to convert to StringRef
in InitNameIndexes.
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:324-326
+ ConstString key_cstr = m_impl.GetCStringAtIndex(item);
+ if (strstr(type_name.GetCString(), key_cstr.GetCString())) {
+count += AppendReplacements(type_name, key_cstr, equivalents);
scott.smith wrote:
> clayborg wrote:
> > StringRef is better to use for modifying simple strings and looking for
> > things. Actually looking at this class it is using ConstString all over the
> > place for putting strings together. For creating strings we should use
> > lldb_private::StreamString. For stripping stuff off and grabbing just part
> > of a string, seeing if a string starts with, ends with, contains, etc,
> > llvm::StringRef. So I would vote to just change it back, or fix the entire
> > class to use lldb_private::StreamString
> I can revert this fragment, but just note this won't affect the number of
> ConstStrings that are generated. m_impl has ConstStrings in it, and
> type_name is a ConstString, so yes they can be converted to StringRef just to
> call StringRef::contains, or we can leave them as ConstString and utilize
> strstr. Or, I can add ConstString::contains so the interface to ConstString
> and StringRef is more similar.
>
turns out this code is unused, filing a separate review to remove it.
Repository:
rL LLVM
https://reviews.llvm.org/D32316
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32503: Remove unused code related to CPlusPlusLanguage::FindEquivalentNames
scott.smith added a comment. In https://reviews.llvm.org/D32503#737953, @tberghammer wrote: > I looked into the history of this code once and my understanding is that > Enrico added this code in > https://github.com/llvm-mirror/lldb/commit/bad9753828b6e0e415e38094bb9627e41d57874c > but it have never been used (at least in upstream). The original commit > message also indicates this. That was four years ago! So we have code that is unused, has no tests, and is not visible via a library (since this header is not in include/lldb/*). The code is also short enough that it can be rewritten if anyone wants it. Ok to delete? Repository: rL LLVM https://reviews.llvm.org/D32503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32503: Remove unused code related to CPlusPlusLanguage::FindEquivalentNames
scott.smith added a comment. In https://reviews.llvm.org/D32503#738243, @zturner wrote: > Code is still present in history, if someone needs this again in the future > they can do some git archaeology to dig it up. Can someone please commit this for me? I don't have access. Thank you! Repository: rL LLVM https://reviews.llvm.org/D32503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32306: Remove lock from ConstString::GetLength
scott.smith added inline comments. Comment at: source/Utility/ConstString.cpp:49 + // pointer, we don't need the lock. const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr); return entry.getKey().size(); zturner wrote: > Why do we even have this function which digs into the `StringMap` internals > rather than just calling existing `StringMap` member functions? Can Can we > just delete `GetStringMapEntryFromKeyData` entirely and use `StringMap::find`? Probably performance. If we have to call Find, then we have to call hash, fault in the appropriate bucket, and then finally return the entry that we already have in hand. Plus we'd need the lock. Repository: rL LLVM https://reviews.llvm.org/D32306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32568: Protect Proces::GetMemoryRegionInfo and ::GetFileLoadAddress with a lock
scott.smith created this revision.
Both routines (on Linux, at least) utilize a cache; protect the cache with a
mutex to allow concurrent callers.
Repository:
rL LLVM
https://reviews.llvm.org/D32568
Files:
source/Plugins/Process/Linux/NativeProcessLinux.cpp
source/Plugins/Process/Linux/NativeProcessLinux.h
source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -1440,7 +1440,6 @@
region_info.Clear();
if (m_supports_memory_region_info != eLazyBoolNo) {
-m_supports_memory_region_info = eLazyBoolYes;
char packet[64];
const int packet_len = ::snprintf(
packet, sizeof(packet), "qMemoryRegionInfo:%" PRIx64, (uint64_t)addr);
@@ -1516,6 +1515,7 @@
// We got an invalid address range back
error.SetErrorString("Server returned invalid range");
}
+ m_supports_memory_region_info = eLazyBoolYes;
} else {
m_supports_memory_region_info = eLazyBoolNo;
}
Index: source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
===
--- source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
+++ source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
@@ -106,6 +106,9 @@
private:
MainLoop::SignalHandleUP m_sigchld_handle;
ArchSpec m_arch;
+
+ // Mutex to protect the memory region cache.
+ std::recursive_mutex m_memory_region_mutex;
LazyBool m_supports_mem_region;
std::vector> m_mem_region_cache;
Index: source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
===
--- source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
+++ source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
@@ -565,6 +565,7 @@
Error NativeProcessNetBSD::GetMemoryRegionInfo(lldb::addr_t load_addr,
MemoryRegionInfo &range_info) {
+ std::lock_guard guard(m_memory_region_mutex);
if (m_supports_mem_region == LazyBool::eLazyBoolNo) {
// We're done.
return Error("unsupported");
Index: source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- source/Plugins/Process/Linux/NativeProcessLinux.h
+++ source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -126,6 +126,8 @@
MainLoop::SignalHandleUP m_sigchld_handle;
ArchSpec m_arch;
+ // Mutex to protect the memory region cache.
+ std::recursive_mutex m_memory_region_mutex;
LazyBool m_supports_mem_region;
std::vector> m_mem_region_cache;
Index: source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -1516,6 +1516,7 @@
Error NativeProcessLinux::GetMemoryRegionInfo(lldb::addr_t load_addr,
MemoryRegionInfo &range_info) {
+ std::lock_guard guard(m_memory_region_mutex);
// FIXME review that the final memory region returned extends to the end of
// the virtual address space,
// with no perms if it is not mapped.
@@ -2221,6 +,7 @@
Error NativeProcessLinux::GetLoadedModuleFileSpec(const char *module_path,
FileSpec &file_spec) {
+ std::lock_guard guard(m_memory_region_mutex);
Error error = PopulateMemoryRegionCache();
if (error.Fail())
return error;
@@ -2240,6 +2242,7 @@
Error NativeProcessLinux::GetFileLoadAddress(const llvm::StringRef &file_name,
lldb::addr_t &load_addr) {
+ std::lock_guard guard(m_memory_region_mutex);
load_addr = LLDB_INVALID_ADDRESS;
Error error = PopulateMemoryRegionCache();
if (error.Fail())
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -1440,7 +1440,6 @@
region_info.Clear();
if (m_supports_memory_region_info != eLazyBoolNo) {
-m_supports_memory_region_info = eLazyBoolYes;
char packet[64];
const int packet_len = ::snprintf(
packet, sizeof(packet), "qMemoryRegionInfo:%" PRIx64, (uint64_t)addr);
@@ -1516,6 +1515,7 @@
// We got an invalid address range back
error.SetErrorString("Server returned invalid range");
}
+ m_supports_memory_region_info = eLazyBoolYes;
} else {
m_supports_memory_reg
[Lldb-commits] [PATCH] D32306: Remove lock from ConstString::GetLength
scott.smith updated this revision to Diff 96841.
scott.smith added a comment.
Use StringMapEntry::GetStringMapEntryFromKeyData instead of ConstString's
version.
Repository:
rL LLVM
https://reviews.llvm.org/D32306
Files:
source/Utility/ConstString.cpp
Index: source/Utility/ConstString.cpp
===
--- source/Utility/ConstString.cpp
+++ source/Utility/ConstString.cpp
@@ -38,14 +38,13 @@
static StringPoolEntryType &
GetStringMapEntryFromKeyData(const char *keyData) {
-char *ptr = const_cast(keyData) - sizeof(StringPoolEntryType);
-return *reinterpret_cast(ptr);
+return StringPoolEntryType::GetStringMapEntryFromKeyData(keyData);
}
- size_t GetConstCStringLength(const char *ccstr) const {
+ static size_t GetConstCStringLength(const char *ccstr) {
if (ccstr != nullptr) {
- const uint8_t h = hash(llvm::StringRef(ccstr));
- llvm::sys::SmartScopedReader rlock(m_string_pools[h].m_mutex);
+ // Since the entry is read only, and we derive the entry entirely from
the
+ // pointer, we don't need the lock.
const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr);
return entry.getKey().size();
}
@@ -218,10 +217,8 @@
if (m_string == rhs.m_string)
return false;
- llvm::StringRef lhs_string_ref(m_string,
- StringPool().GetConstCStringLength(m_string));
- llvm::StringRef rhs_string_ref(
- rhs.m_string, StringPool().GetConstCStringLength(rhs.m_string));
+ llvm::StringRef lhs_string_ref(GetStringRef());
+ llvm::StringRef rhs_string_ref(rhs.GetStringRef());
// If both have valid C strings, then return the comparison
if (lhs_string_ref.data() && rhs_string_ref.data())
@@ -240,7 +237,7 @@
}
size_t ConstString::GetLength() const {
- return StringPool().GetConstCStringLength(m_string);
+ return Pool::GetConstCStringLength(m_string);
}
bool ConstString::Equals(const ConstString &lhs, const ConstString &rhs,
@@ -255,10 +252,8 @@
return false;
// perform case insensitive equality test
- llvm::StringRef lhs_string_ref(
- lhs.m_string, StringPool().GetConstCStringLength(lhs.m_string));
- llvm::StringRef rhs_string_ref(
- rhs.m_string, StringPool().GetConstCStringLength(rhs.m_string));
+ llvm::StringRef lhs_string_ref(lhs.GetStringRef());
+ llvm::StringRef rhs_string_ref(rhs.GetStringRef());
return lhs_string_ref.equals_lower(rhs_string_ref);
}
@@ -270,10 +265,8 @@
if (lhs_cstr == rhs_cstr)
return 0;
if (lhs_cstr && rhs_cstr) {
-llvm::StringRef lhs_string_ref(
-lhs_cstr, StringPool().GetConstCStringLength(lhs_cstr));
-llvm::StringRef rhs_string_ref(
-rhs_cstr, StringPool().GetConstCStringLength(rhs_cstr));
+llvm::StringRef lhs_string_ref(lhs.GetStringRef());
+llvm::StringRef rhs_string_ref(rhs.GetStringRef());
if (case_sensitive) {
return lhs_string_ref.compare(rhs_string_ref);
Index: source/Utility/ConstString.cpp
===
--- source/Utility/ConstString.cpp
+++ source/Utility/ConstString.cpp
@@ -38,14 +38,13 @@
static StringPoolEntryType &
GetStringMapEntryFromKeyData(const char *keyData) {
-char *ptr = const_cast(keyData) - sizeof(StringPoolEntryType);
-return *reinterpret_cast(ptr);
+return StringPoolEntryType::GetStringMapEntryFromKeyData(keyData);
}
- size_t GetConstCStringLength(const char *ccstr) const {
+ static size_t GetConstCStringLength(const char *ccstr) {
if (ccstr != nullptr) {
- const uint8_t h = hash(llvm::StringRef(ccstr));
- llvm::sys::SmartScopedReader rlock(m_string_pools[h].m_mutex);
+ // Since the entry is read only, and we derive the entry entirely from the
+ // pointer, we don't need the lock.
const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr);
return entry.getKey().size();
}
@@ -218,10 +217,8 @@
if (m_string == rhs.m_string)
return false;
- llvm::StringRef lhs_string_ref(m_string,
- StringPool().GetConstCStringLength(m_string));
- llvm::StringRef rhs_string_ref(
- rhs.m_string, StringPool().GetConstCStringLength(rhs.m_string));
+ llvm::StringRef lhs_string_ref(GetStringRef());
+ llvm::StringRef rhs_string_ref(rhs.GetStringRef());
// If both have valid C strings, then return the comparison
if (lhs_string_ref.data() && rhs_string_ref.data())
@@ -240,7 +237,7 @@
}
size_t ConstString::GetLength() const {
- return StringPool().GetConstCStringLength(m_string);
+ return Pool::GetConstCStringLength(m_string);
}
bool ConstString::Equals(const ConstString &lhs, const ConstString &rhs,
@@ -255,10 +252,8 @@
return false;
// perform case insensitive equality test
- llvm::StringRef lhs_string_ref(
- lhs.m_string, StringPool().GetConstCStringLength(lhs.m_string));
- llvm::StringRef rhs_st
[Lldb-commits] [PATCH] D32306: Remove lock from ConstString::GetLength
scott.smith marked 3 inline comments as done. scott.smith added inline comments. Comment at: source/Utility/ConstString.cpp:49 + // pointer, we don't need the lock. const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr); return entry.getKey().size(); labath wrote: > labath wrote: > > scott.smith wrote: > > > zturner wrote: > > > > Why do we even have this function which digs into the `StringMap` > > > > internals rather than just calling existing `StringMap` member > > > > functions? Can Can we just delete `GetStringMapEntryFromKeyData` > > > > entirely and use `StringMap::find`? > > > Probably performance. If we have to call Find, then we have to call > > > hash, fault in the appropriate bucket, and then finally return the entry > > > that we already have in hand. Plus we'd need the lock. > > > > > > Can we just delete GetStringMapEntryFromKeyData entirely and use > > > StringMap::find? > > Unfortunately, I don't think that's possible. `StringMap::find` expects a > > StringRef. In order to construct that, we need to know the length of the > > string, and we're back where we started :( > > > > In reality, this is doing a very different operation than find (which takes > > a random string and checks whether it's in the map) -- this takes a string > > which we know to be in the map and get its size. > > > > It will take some rearchitecting of the ConstString class to get rid of > > this hack. Probably it could be fixed by ConstString storing a > > StringMap::iterator instead of the raw pointer. In any case, that seems out > > of scope of this change. > Cool, I didn't notice that one when looking for it. I guess at this point we > can just delete our copy of `GetStringMapEntryFromKeyData` completely and > call the StringPool's version instead. It will push a lot of lines past the 80 char limit. Do you want me to make that change? If not, can you submit this one since I do not have commit access? Thanks! Repository: rL LLVM https://reviews.llvm.org/D32306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32568: Protect Proces::GetMemoryRegionInfo and ::GetFileLoadAddress with a lock
scott.smith added a comment. In https://reviews.llvm.org/D32568#739190, @labath wrote: > This is not necessary. NativeProcess classes are only used in lldb-server, > which is completely single threaded. If you want to change that, then we > should start with a discussion of what you intend to achieve. Let me post the other two changes to start a broader discussion. We can center the conversation around whether/how to prime the caches; the other two changes naturally follow from that. Repository: rL LLVM https://reviews.llvm.org/D32568 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32597: Initiate loading of shared libraries in parallel
scott.smith created this revision.
This change forks a thread for each shared library, so that as much work as
possible can be done in parallel.
Repository:
rL LLVM
https://reviews.llvm.org/D32597
Files:
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
Index: source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
===
--- source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
+++ source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
@@ -66,6 +66,10 @@
uint32_t GetPluginVersion() override;
protected:
+ /// Mutex to protect various global variables during parallel shared library
+ /// loading.
+ std::recursive_mutex m_mutex;
+
/// Runtime linker rendezvous structure.
DYLDRendezvous m_rendezvous;
Index: source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
===
--- source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -29,6 +29,8 @@
#include "lldb/Utility/Log.h"
// C++ Includes
+#include
+
// C Includes
using namespace lldb;
@@ -195,6 +197,7 @@
}
void DynamicLoaderPOSIXDYLD::DidLaunch() {
+ std::lock_guard guard(m_mutex);
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
if (log)
log->Printf("DynamicLoaderPOSIXDYLD::%s()", __FUNCTION__);
@@ -228,17 +231,20 @@
addr_t link_map_addr,
addr_t base_addr,
bool base_addr_is_offset) {
+ std::lock_guard guard(m_mutex);
m_loaded_modules[module] = link_map_addr;
UpdateLoadedSectionsCommon(module, base_addr, base_addr_is_offset);
}
void DynamicLoaderPOSIXDYLD::UnloadSections(const ModuleSP module) {
+ std::lock_guard guard(m_mutex);
m_loaded_modules.erase(module);
UnloadSectionsCommon(module);
}
void DynamicLoaderPOSIXDYLD::ProbeEntry() {
+ std::lock_guard guard(m_mutex);
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
const addr_t entry = GetEntryPoint();
@@ -329,6 +335,7 @@
}
void DynamicLoaderPOSIXDYLD::SetRendezvousBreakpoint() {
+ std::lock_guard guard(m_mutex);
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
addr_t break_addr = m_rendezvous.GetBreakAddress();
@@ -372,6 +379,7 @@
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
DynamicLoaderPOSIXDYLD *const dyld_instance =
static_cast(baton);
+ std::lock_guard guard(dyld_instance->m_mutex);
if (log)
log->Printf("DynamicLoaderPOSIXDYLD::%s called for pid %" PRIu64,
__FUNCTION__,
@@ -393,6 +401,7 @@
}
void DynamicLoaderPOSIXDYLD::RefreshModules() {
+ std::lock_guard guard(m_mutex);
if (!m_rendezvous.Resolve())
return;
@@ -437,6 +446,7 @@
ThreadPlanSP
DynamicLoaderPOSIXDYLD::GetStepThroughTrampolinePlan(Thread &thread,
bool stop) {
+ std::lock_guard guard(m_mutex);
ThreadPlanSP thread_plan_sp;
StackFrame *frame = thread.GetStackFrameAtIndex(0).get();
@@ -517,24 +527,40 @@
m_process->PrefetchModuleSpecs(
module_names, m_process->GetTarget().GetArchitecture().GetTriple());
+ struct loader {
+std::thread t;
+ModuleSP m;
+DYLDRendezvous::iterator I;
+ };
+ std::list loaders;
for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I) {
-ModuleSP module_sp =
-LoadModuleAtAddress(I->file_spec, I->link_addr, I->base_addr, true);
-if (module_sp.get()) {
- module_list.Append(module_sp);
+loaders.push_back(loader());
+auto * last = &loaders.back();
+last->I = I;
+last->t = std::thread([this, last]()
+{
+ last->m = LoadModuleAtAddress(
+last->I->file_spec, last->I->link_addr, last->I->base_addr, true);
+});
+ }
+ for (auto & l : loaders) {
+l.t.join();
+if (l.m.get()) {
+ module_list.Append(l.m);
} else {
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
if (log)
log->Printf(
"DynamicLoaderPOSIXDYLD::%s failed loading module %s at 0x%" PRIx64,
-__FUNCTION__, I->file_spec.GetCString(), I->base_addr);
+__FUNCTION__, l.I->file_spec.GetCString(), l.I->base_addr);
}
}
m_process->GetTarget().ModulesDidLoad(module_list);
}
addr_t DynamicLoaderPOSIXDYLD::ComputeLoadOffset() {
+ std::lock_guard guard(m_mutex);
addr_t virt_entry;
if (m_load_offset != LLDB_INVALID_ADDRESS)
@@ -561,13 +587,15 @@
}
void DynamicLoaderPOSIXDYLD::EvalVdsoStatus() {
+ std::lock_guard guard(m_mutex);
AuxVector::iterator I = m_auxv->FindEntry(AuxVector::AT_SYSINFO_EHDR);
if (I != m_
[Lldb-commits] [PATCH] D32597: Initiate loading of shared libraries in parallel
scott.smith added a comment. 1. This change requires https://reviews.llvm.org/D32568 for correctness. 2. I think the use of std::thread should be replaced by a custom TaskPool, or else executables with thousands of shared libraries may have a problem. A separate TaskPool is necessary to prevent recursive TaskPool use. Repository: rL LLVM https://reviews.llvm.org/D32597 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism
scott.smith created this revision.
Loading a shared library can require a large amount of work; rather than do
that serially for each library, provide a mechanism to do some amount of
priming before hand.
Repository:
rL LLVM
https://reviews.llvm.org/D32598
Files:
include/lldb/Core/Module.h
include/lldb/Symbol/SymbolFile.h
include/lldb/Symbol/Symtab.h
source/Core/Module.cpp
source/Core/ModuleList.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
source/Symbol/SymbolFile.cpp
source/Symbol/Symtab.cpp
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -425,6 +425,12 @@
}
}
+void Symtab::PrimeCaches()
+{
+ std::lock_guard guard(m_mutex);
+ InitNameIndexes();
+}
+
void Symtab::AppendSymbolNamesToMap(const IndexCollection &indexes,
bool add_demangled, bool add_mangled,
NameToIndexMap &name_to_index_map) const {
Index: source/Symbol/SymbolFile.cpp
===
--- source/Symbol/SymbolFile.cpp
+++ source/Symbol/SymbolFile.cpp
@@ -21,6 +21,10 @@
using namespace lldb_private;
+void SymbolFile::PrimeCaches() {
+ // No-op for most implementations.
+}
+
SymbolFile *SymbolFile::FindPlugin(ObjectFile *obj_file) {
std::unique_ptr best_symfile_ap;
if (obj_file != nullptr) {
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -226,6 +226,8 @@
const lldb_private::ConstString &name,
const lldb_private::CompilerDeclContext *parent_decl_ctx) override;
+ void PrimeCaches() override;
+
//--
// PluginInterface protocol
//--
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1917,6 +1917,12 @@
return sc_list.GetSize() - prev_size;
}
+void SymbolFileDWARF::PrimeCaches() {
+ std::lock_guard guard(
+ GetObjectFile()->GetModule()->GetMutex());
+ Index();
+}
+
void SymbolFileDWARF::Index() {
if (m_indexed)
return;
Index: source/Core/ModuleList.cpp
===
--- source/Core/ModuleList.cpp
+++ source/Core/ModuleList.cpp
@@ -103,6 +103,11 @@
void ModuleList::AppendImpl(const ModuleSP &module_sp, bool use_notifier) {
if (module_sp) {
+// Do this outside of the lock for parallelism, in case we're called from a
+// separate thread.
+if (use_notifier && m_notifier)
+ module_sp->PrimeCaches();
+
std::lock_guard guard(m_modules_mutex);
m_modules.push_back(module_sp);
if (use_notifier && m_notifier)
Index: source/Core/Module.cpp
===
--- source/Core/Module.cpp
+++ source/Core/Module.cpp
@@ -140,7 +140,7 @@
const bool mandatory = true;
ModuleList::RemoveOrphanSharedModules(mandatory);
}
-
+
void
DumpModuleInfo (void)
{
@@ -150,15 +150,15 @@
printf ("%s: %" PRIu64 " modules:\n", LLVM_PRETTY_FUNCTION, (uint64_t)count);
for (size_t i = 0; i < count; ++i)
{
-
+
StreamString strm;
Module *module = modules[i];
const bool in_shared_module_list = ModuleList::ModuleIsInCache (module);
module->GetDescription(&strm, eDescriptionLevelFull);
-printf ("%p: shared = %i, ref_count = %3u, module = %s\n",
-module,
+printf ("%p: shared = %i, ref_count = %3u, module = %s\n",
+module,
in_shared_module_list,
-(uint32_t)module->use_count(),
+(uint32_t)module->use_count(),
strm.GetString().c_str());
}
}
@@ -1432,6 +1432,22 @@
return sc_list.GetSize() - initial_size;
}
+void Module::PrimeCaches() {
+ std::lock_guard guard(m_mutex);
+ SymbolVendor * sym_vendor = GetSymbolVendor();
+ if (!sym_vendor) {
+return;
+ }
+ // Prime the symbol file first, since it adds symbols to the symbol table.
+ if (SymbolFile *symbol_file = sym_vendor->GetSymbolFile()) {
+symbol_file->PrimeCaches();
+ }
+ // Now we can prime the symbol table.
+ if (Symtab * symtab = sym_vendor->GetSymtab()) {
+symtab->PrimeCaches();
+ }
+}
+
void Module::SetSymbolFileFileSpec(const FileSpec &file) {
if (!file.Exists())
return;
Index: include/lldb/S
[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism
scott.smith added a comment.
Here's the controversial patch. It has been brought up that the intent is to
not load symbols before they are needed, but at least in my experience this
patch has no effect on performance when running:
lldb -b -o run /path/to/myprogram
where myprogram has main(){return 0;} and links in a whole lot of libraries.
My platform is Ubuntu 14.04; are there other systems where symbol processing is
actually delayed? Maybe on those systems PrimeCaches() can be a no-op to not
impact performance?
This patch requires https://reviews.llvm.org/D32597 in order to actually
improve performance, but does not require it for correctness.
Repository:
rL LLVM
https://reviews.llvm.org/D32598
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key
scott.smith added a comment. Can I get a re-review on this? Thanks. Repository: rL LLVM https://reviews.llvm.org/D32316 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32568: Protect Proces::GetMemoryRegionInfo and ::GetFileLoadAddress with a lock
scott.smith added a comment. In https://reviews.llvm.org/D32568#739723, @labath wrote: > All your other changes are client-side, so still think this will not matter, > but I'll take a look. :) Interesting. Maybe this only matters in the context of unit tests? I was still getting crashes without this change, but now I'm not (I've since git pull'd and switched to another machine). I'll keep investigating. Repository: rL LLVM https://reviews.llvm.org/D32568 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism
scott.smith added a comment. In https://reviews.llvm.org/D32598#739779, @clayborg wrote: > Making an empty main program and saying I see no difference is not enough > testing to enable this. It's not quite an empty main program; it links in 40+ shared libraries with 2M+ symbols. The point of having main() just return 0 is so I could compare the performance of "b main; run" vs "run" without taking program execution into account, but just the amount of work lldb does. But as it turns out, I still managed to screw that up. I'm now seeing a difference: WITHOUT CHANGE: lldb -b -o 'b main' -o 'run' xyz: real 0m15.248s user 0m24.632s sys 0m3.839s lldb -b -o 'run' xyz: real0m11.540s user0m9.692s sys 0m0.892s WITH CHANGE: real 0m9.747s user 0m29.507s sys 0m10.531s (timings are reasonably representative, but they do change a bit run to run) > I also don't see the benefit of this path. It improves lldb startup time significantly. Our use cases seem centered around using symbols, so for us, delaying loading of symbols does not help, it just moves when we wait to another point. > When LLDB is used for symbolication, it might never actually load any debug > info or symbols from modules that are added to a target. Also if there is any > DWARF that doesn't have the Apple accelerator tables then the DWARF manually > indexes itself by spawning threads that will index a compile unit per thread > using a thread pool.. So now if we load all symbols at once, using NUM_CORES > threads, and then each DWARF file spins off NUM_CORES threads to index the > DWARF we can probably bring the machine to the brink? No, because it uses a global TaskPool, so regardless of how many users there are, there will only be NUM_CORES threads. Note my comments in https://reviews.llvm.org/D32597 about changing TaskPool so it will be file-local instead of global, so TaskPool workers can spawn other TaskPool work. Repository: rL LLVM https://reviews.llvm.org/D32598 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism
scott.smith added a comment. In https://reviews.llvm.org/D32598#739804, @jingham wrote: > Instead of having the cache priming be determined by platform or something > like that, it would be better to add a setting (on the target level seems > right) that controls this. I think you are right that in most common usage, > there's going to be an unrestricted query that will end up looking through > all the symbols, and in that case, having ingested them in parallel would be > a win. So the correct default would be to do this eagerly. Define "a setting." A method on class Target? A parameter when constructing class Target? Or something via the command line, or lldbinit? Repository: rL LLVM https://reviews.llvm.org/D32598 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism
scott.smith updated this revision to Diff 96976.
scott.smith added a comment.
Added setting. Verified that:
settings set target.cache-priming off
works as expected.
Repository:
rL LLVM
https://reviews.llvm.org/D32598
Files:
include/lldb/Core/Module.h
include/lldb/Symbol/SymbolFile.h
include/lldb/Symbol/Symtab.h
include/lldb/Target/Target.h
source/Core/Module.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
source/Symbol/SymbolFile.cpp
source/Symbol/Symtab.cpp
source/Target/Target.cpp
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1870,6 +1870,11 @@
}
}
+// Prime caches outside of any lock, so hopefully we can do this for
+// each library in parallel.
+if (GetCachePriming())
+ module_sp->PrimeCaches();
+
if (old_module_sp &&
m_images.GetIndexForModule(old_module_sp.get()) !=
LLDB_INVALID_INDEX32) {
@@ -3277,6 +3282,8 @@
{"detach-on-error", OptionValue::eTypeBoolean, false, true, nullptr,
nullptr, "debugserver will detach (rather than killing) a process if it "
"loses connection with lldb."},
+{"cache-priming", OptionValue::eTypeBoolean, false, true, nullptr, nullptr,
+ "Enable loading of symbol tables before they are needed."},
{"disable-aslr", OptionValue::eTypeBoolean, false, true, nullptr, nullptr,
"Disable Address Space Layout Randomization (ASLR)"},
{"disable-stdio", OptionValue::eTypeBoolean, false, false, nullptr, nullptr,
@@ -3379,6 +3386,7 @@
ePropertyOutputPath,
ePropertyErrorPath,
ePropertyDetachOnError,
+ ePropertyCachePriming,
ePropertyDisableASLR,
ePropertyDisableSTDIO,
ePropertyInlineStrategy,
@@ -3641,6 +3649,17 @@
return m_collection_sp->SetPropertyAtIndexAsEnumeration(nullptr, idx, d);
}
+bool TargetProperties::GetCachePriming() const {
+ const uint32_t idx = ePropertyCachePriming;
+ return m_collection_sp->GetPropertyAtIndexAsBoolean(
+ nullptr, idx, g_properties[idx].default_uint_value != 0);
+}
+
+void TargetProperties::SetCachePriming(bool b) {
+ const uint32_t idx = ePropertyCachePriming;
+ m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+}
+
bool TargetProperties::GetDisableASLR() const {
const uint32_t idx = ePropertyDisableASLR;
return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -425,6 +425,11 @@
}
}
+void Symtab::PrimeCaches() {
+ std::lock_guard guard(m_mutex);
+ InitNameIndexes();
+}
+
void Symtab::AppendSymbolNamesToMap(const IndexCollection &indexes,
bool add_demangled, bool add_mangled,
NameToIndexMap &name_to_index_map) const {
Index: source/Symbol/SymbolFile.cpp
===
--- source/Symbol/SymbolFile.cpp
+++ source/Symbol/SymbolFile.cpp
@@ -21,6 +21,10 @@
using namespace lldb_private;
+void SymbolFile::PrimeCaches() {
+ // No-op for most implementations.
+}
+
SymbolFile *SymbolFile::FindPlugin(ObjectFile *obj_file) {
std::unique_ptr best_symfile_ap;
if (obj_file != nullptr) {
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -226,6 +226,8 @@
const lldb_private::ConstString &name,
const lldb_private::CompilerDeclContext *parent_decl_ctx) override;
+ void PrimeCaches() override;
+
//--
// PluginInterface protocol
//--
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1917,6 +1917,12 @@
return sc_list.GetSize() - prev_size;
}
+void SymbolFileDWARF::PrimeCaches() {
+ std::lock_guard guard(
+ GetObjectFile()->GetModule()->GetMutex());
+ Index();
+}
+
void SymbolFileDWARF::Index() {
if (m_indexed)
return;
Index: source/Core/Module.cpp
===
--- source/Core/Module.cpp
+++ source/Core/Module.cpp
@@ -1432,6 +1432,22 @@
return sc_list.GetSize() - initial_size;
}
+void Module::PrimeCaches() {
+ std::lock_guard guard(m_mutex);
+ SymbolVendor * sym_vendor = GetSymbolVendor();
+ if (!sym_vendor) {
+return;
+ }
+ // Prime the symbol file first, since it adds symbol
[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism
scott.smith updated this revision to Diff 96990.
scott.smith added a comment.
1. Rename to preload-symbols / PreloadSymbols()
2. Modify an existing test to run with and without symbol preloading.
Repository:
rL LLVM
https://reviews.llvm.org/D32598
Files:
include/lldb/Core/Module.h
include/lldb/Symbol/SymbolFile.h
include/lldb/Symbol/Symtab.h
include/lldb/Target/Target.h
packages/Python/lldbsuite/test/lang/c/shared_lib/TestSharedLib.py
source/Core/Module.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
source/Symbol/SymbolFile.cpp
source/Symbol/Symtab.cpp
source/Target/Target.cpp
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1870,6 +1870,11 @@
}
}
+// Preload symbols outside of any lock, so hopefully we can do this for
+// each library in parallel.
+if (GetPreloadSymbols())
+ module_sp->PreloadSymbols();
+
if (old_module_sp &&
m_images.GetIndexForModule(old_module_sp.get()) !=
LLDB_INVALID_INDEX32) {
@@ -3277,6 +3282,8 @@
{"detach-on-error", OptionValue::eTypeBoolean, false, true, nullptr,
nullptr, "debugserver will detach (rather than killing) a process if it "
"loses connection with lldb."},
+{"preload-symbols", OptionValue::eTypeBoolean, false, true, nullptr, nullptr,
+ "Enable loading of symbol tables before they are needed."},
{"disable-aslr", OptionValue::eTypeBoolean, false, true, nullptr, nullptr,
"Disable Address Space Layout Randomization (ASLR)"},
{"disable-stdio", OptionValue::eTypeBoolean, false, false, nullptr, nullptr,
@@ -3379,6 +3386,7 @@
ePropertyOutputPath,
ePropertyErrorPath,
ePropertyDetachOnError,
+ ePropertyPreloadSymbols,
ePropertyDisableASLR,
ePropertyDisableSTDIO,
ePropertyInlineStrategy,
@@ -3641,6 +3649,17 @@
return m_collection_sp->SetPropertyAtIndexAsEnumeration(nullptr, idx, d);
}
+bool TargetProperties::GetPreloadSymbols() const {
+ const uint32_t idx = ePropertyPreloadSymbols;
+ return m_collection_sp->GetPropertyAtIndexAsBoolean(
+ nullptr, idx, g_properties[idx].default_uint_value != 0);
+}
+
+void TargetProperties::SetPreloadSymbols(bool b) {
+ const uint32_t idx = ePropertyPreloadSymbols;
+ m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+}
+
bool TargetProperties::GetDisableASLR() const {
const uint32_t idx = ePropertyDisableASLR;
return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -425,6 +425,11 @@
}
}
+void Symtab::PreloadSymbols() {
+ std::lock_guard guard(m_mutex);
+ InitNameIndexes();
+}
+
void Symtab::AppendSymbolNamesToMap(const IndexCollection &indexes,
bool add_demangled, bool add_mangled,
NameToIndexMap &name_to_index_map) const {
Index: source/Symbol/SymbolFile.cpp
===
--- source/Symbol/SymbolFile.cpp
+++ source/Symbol/SymbolFile.cpp
@@ -21,6 +21,10 @@
using namespace lldb_private;
+void SymbolFile::PreloadSymbols() {
+ // No-op for most implementations.
+}
+
SymbolFile *SymbolFile::FindPlugin(ObjectFile *obj_file) {
std::unique_ptr best_symfile_ap;
if (obj_file != nullptr) {
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -226,6 +226,8 @@
const lldb_private::ConstString &name,
const lldb_private::CompilerDeclContext *parent_decl_ctx) override;
+ void PreloadSymbols() override;
+
//--
// PluginInterface protocol
//--
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1917,6 +1917,12 @@
return sc_list.GetSize() - prev_size;
}
+void SymbolFileDWARF::PreloadSymbols() {
+ std::lock_guard guard(
+ GetObjectFile()->GetModule()->GetMutex());
+ Index();
+}
+
void SymbolFileDWARF::Index() {
if (m_indexed)
return;
Index: source/Core/Module.cpp
===
--- source/Core/Module.cpp
+++ source/Core/Module.cpp
@@ -1432,6 +1432,22 @@
return sc_list.GetSize() - initial_size;
}
+void Module::PreloadSymbols() {
+ std::lock_guard guard(m_mutex);
+ SymbolVen
[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism
scott.smith updated this revision to Diff 96991.
scott.smith added a comment.
Fix default param to setup_common so that we actually test both paths.
Repository:
rL LLVM
https://reviews.llvm.org/D32598
Files:
include/lldb/Core/Module.h
include/lldb/Symbol/SymbolFile.h
include/lldb/Symbol/Symtab.h
include/lldb/Target/Target.h
packages/Python/lldbsuite/test/lang/c/shared_lib/TestSharedLib.py
source/Core/Module.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
source/Symbol/SymbolFile.cpp
source/Symbol/Symtab.cpp
source/Target/Target.cpp
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1870,6 +1870,11 @@
}
}
+// Preload symbols outside of any lock, so hopefully we can do this for
+// each library in parallel.
+if (GetPreloadSymbols())
+ module_sp->PreloadSymbols();
+
if (old_module_sp &&
m_images.GetIndexForModule(old_module_sp.get()) !=
LLDB_INVALID_INDEX32) {
@@ -3277,6 +3282,8 @@
{"detach-on-error", OptionValue::eTypeBoolean, false, true, nullptr,
nullptr, "debugserver will detach (rather than killing) a process if it "
"loses connection with lldb."},
+{"preload-symbols", OptionValue::eTypeBoolean, false, true, nullptr, nullptr,
+ "Enable loading of symbol tables before they are needed."},
{"disable-aslr", OptionValue::eTypeBoolean, false, true, nullptr, nullptr,
"Disable Address Space Layout Randomization (ASLR)"},
{"disable-stdio", OptionValue::eTypeBoolean, false, false, nullptr, nullptr,
@@ -3379,6 +3386,7 @@
ePropertyOutputPath,
ePropertyErrorPath,
ePropertyDetachOnError,
+ ePropertyPreloadSymbols,
ePropertyDisableASLR,
ePropertyDisableSTDIO,
ePropertyInlineStrategy,
@@ -3641,6 +3649,17 @@
return m_collection_sp->SetPropertyAtIndexAsEnumeration(nullptr, idx, d);
}
+bool TargetProperties::GetPreloadSymbols() const {
+ const uint32_t idx = ePropertyPreloadSymbols;
+ return m_collection_sp->GetPropertyAtIndexAsBoolean(
+ nullptr, idx, g_properties[idx].default_uint_value != 0);
+}
+
+void TargetProperties::SetPreloadSymbols(bool b) {
+ const uint32_t idx = ePropertyPreloadSymbols;
+ m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+}
+
bool TargetProperties::GetDisableASLR() const {
const uint32_t idx = ePropertyDisableASLR;
return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -425,6 +425,11 @@
}
}
+void Symtab::PreloadSymbols() {
+ std::lock_guard guard(m_mutex);
+ InitNameIndexes();
+}
+
void Symtab::AppendSymbolNamesToMap(const IndexCollection &indexes,
bool add_demangled, bool add_mangled,
NameToIndexMap &name_to_index_map) const {
Index: source/Symbol/SymbolFile.cpp
===
--- source/Symbol/SymbolFile.cpp
+++ source/Symbol/SymbolFile.cpp
@@ -21,6 +21,10 @@
using namespace lldb_private;
+void SymbolFile::PreloadSymbols() {
+ // No-op for most implementations.
+}
+
SymbolFile *SymbolFile::FindPlugin(ObjectFile *obj_file) {
std::unique_ptr best_symfile_ap;
if (obj_file != nullptr) {
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -226,6 +226,8 @@
const lldb_private::ConstString &name,
const lldb_private::CompilerDeclContext *parent_decl_ctx) override;
+ void PreloadSymbols() override;
+
//--
// PluginInterface protocol
//--
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1917,6 +1917,12 @@
return sc_list.GetSize() - prev_size;
}
+void SymbolFileDWARF::PreloadSymbols() {
+ std::lock_guard guard(
+ GetObjectFile()->GetModule()->GetMutex());
+ Index();
+}
+
void SymbolFileDWARF::Index() {
if (m_indexed)
return;
Index: source/Core/Module.cpp
===
--- source/Core/Module.cpp
+++ source/Core/Module.cpp
@@ -1432,6 +1432,22 @@
return sc_list.GetSize() - initial_size;
}
+void Module::PreloadSymbols() {
+ std::lock_guard guard(m_mutex);
+ SymbolVendor * sym_vendor = GetSymbolVendor();
+ if (!s
[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism
scott.smith updated this revision to Diff 97002.
scott.smith added a comment.
Add test to print expression (calls func, hence resolves symbols). Also better
parameterize the common test to reduce code duplication.
Repository:
rL LLVM
https://reviews.llvm.org/D32598
Files:
include/lldb/Core/Module.h
include/lldb/Symbol/SymbolFile.h
include/lldb/Symbol/Symtab.h
include/lldb/Target/Target.h
packages/Python/lldbsuite/test/lang/c/shared_lib/TestSharedLib.py
source/Core/Module.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
source/Symbol/SymbolFile.cpp
source/Symbol/Symtab.cpp
source/Target/Target.cpp
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1870,6 +1870,11 @@
}
}
+// Preload symbols outside of any lock, so hopefully we can do this for
+// each library in parallel.
+if (GetPreloadSymbols())
+ module_sp->PreloadSymbols();
+
if (old_module_sp &&
m_images.GetIndexForModule(old_module_sp.get()) !=
LLDB_INVALID_INDEX32) {
@@ -3277,6 +3282,8 @@
{"detach-on-error", OptionValue::eTypeBoolean, false, true, nullptr,
nullptr, "debugserver will detach (rather than killing) a process if it "
"loses connection with lldb."},
+{"preload-symbols", OptionValue::eTypeBoolean, false, true, nullptr, nullptr,
+ "Enable loading of symbol tables before they are needed."},
{"disable-aslr", OptionValue::eTypeBoolean, false, true, nullptr, nullptr,
"Disable Address Space Layout Randomization (ASLR)"},
{"disable-stdio", OptionValue::eTypeBoolean, false, false, nullptr, nullptr,
@@ -3379,6 +3386,7 @@
ePropertyOutputPath,
ePropertyErrorPath,
ePropertyDetachOnError,
+ ePropertyPreloadSymbols,
ePropertyDisableASLR,
ePropertyDisableSTDIO,
ePropertyInlineStrategy,
@@ -3641,6 +3649,17 @@
return m_collection_sp->SetPropertyAtIndexAsEnumeration(nullptr, idx, d);
}
+bool TargetProperties::GetPreloadSymbols() const {
+ const uint32_t idx = ePropertyPreloadSymbols;
+ return m_collection_sp->GetPropertyAtIndexAsBoolean(
+ nullptr, idx, g_properties[idx].default_uint_value != 0);
+}
+
+void TargetProperties::SetPreloadSymbols(bool b) {
+ const uint32_t idx = ePropertyPreloadSymbols;
+ m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+}
+
bool TargetProperties::GetDisableASLR() const {
const uint32_t idx = ePropertyDisableASLR;
return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -425,6 +425,11 @@
}
}
+void Symtab::PreloadSymbols() {
+ std::lock_guard guard(m_mutex);
+ InitNameIndexes();
+}
+
void Symtab::AppendSymbolNamesToMap(const IndexCollection &indexes,
bool add_demangled, bool add_mangled,
NameToIndexMap &name_to_index_map) const {
Index: source/Symbol/SymbolFile.cpp
===
--- source/Symbol/SymbolFile.cpp
+++ source/Symbol/SymbolFile.cpp
@@ -21,6 +21,10 @@
using namespace lldb_private;
+void SymbolFile::PreloadSymbols() {
+ // No-op for most implementations.
+}
+
SymbolFile *SymbolFile::FindPlugin(ObjectFile *obj_file) {
std::unique_ptr best_symfile_ap;
if (obj_file != nullptr) {
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -226,6 +226,8 @@
const lldb_private::ConstString &name,
const lldb_private::CompilerDeclContext *parent_decl_ctx) override;
+ void PreloadSymbols() override;
+
//--
// PluginInterface protocol
//--
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1917,6 +1917,12 @@
return sc_list.GetSize() - prev_size;
}
+void SymbolFileDWARF::PreloadSymbols() {
+ std::lock_guard guard(
+ GetObjectFile()->GetModule()->GetMutex());
+ Index();
+}
+
void SymbolFileDWARF::Index() {
if (m_indexed)
return;
Index: source/Core/Module.cpp
===
--- source/Core/Module.cpp
+++ source/Core/Module.cpp
@@ -1432,6 +1432,22 @@
return sc_list.GetSize() - initial_size;
}
+void Module::PreloadSymbols() {
+ std::lock_guard guard(m_
[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism
scott.smith added a comment. Can someone commit this for me? Thanks! Repository: rL LLVM https://reviews.llvm.org/D32598 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32626: Make the ELF symbol demangling loop order independent
scott.smith created this revision.
Currently, the loop will insert entries into the class_contexts set, and then
use the absence or presence to affect decisions made by later iterations of the
same loop.
In order to support parallelizing the loop, this change moves those decisions
to always occur after the main loop, instead of sometimes occurring after the
loop.
Repository:
rL LLVM
https://reviews.llvm.org/D32626
Files:
source/Symbol/Symtab.cpp
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -310,22 +310,16 @@
m_method_to_index.Append(entry);
} else {
if (const_context && const_context[0]) {
- if (class_contexts.find(const_context) !=
- class_contexts.end()) {
-// The current decl context is in our "class_contexts"
which
-// means
-// this is a method on a class
-m_method_to_index.Append(entry);
- } else {
-// We don't know if this is a function basename or a
method,
-// so put it into a temporary collection so once we are
done
-// we can look in class_contexts to see if each entry is a
-// class
-// or just a function and will put any remaining items into
-// m_method_to_index or m_basename_to_index as needed
-mangled_name_to_index.Append(entry);
-symbol_contexts[entry.value] = const_context;
- }
+ // We don't know if this is a function basename or a method,
+ // so put it into a temporary collection so once we are done
+ // we can look in class_contexts to see if each entry is a
+ // class
+ // or just a function and will put any remaining items into
+ // m_method_to_index or m_basename_to_index as needed
+ mangled_name_to_index.Append(entry);
+ symbol_contexts[entry.value] = const_context;
+ // Note this is common to both paths, though
+ m_method_to_index.Append(entry);
} else {
// No context for this function so this has to be a basename
m_basename_to_index.Append(entry);
@@ -373,26 +367,19 @@
}
}
-size_t count;
-if (!mangled_name_to_index.IsEmpty()) {
- count = mangled_name_to_index.GetSize();
- for (size_t i = 0; i < count; ++i) {
-if (mangled_name_to_index.GetValueAtIndex(i, entry.value)) {
- entry.cstring = mangled_name_to_index.GetCStringAtIndex(i);
- if (symbol_contexts[entry.value] &&
- class_contexts.find(symbol_contexts[entry.value]) !=
- class_contexts.end()) {
-m_method_to_index.Append(entry);
- } else {
-// If we got here, we have something that had a context (was inside
-// a namespace or class)
-// yet we don't know if the entry
-m_method_to_index.Append(entry);
-m_basename_to_index.Append(entry);
- }
+size_t count = mangled_name_to_index.GetSize();
+for (size_t i = 0; i < count; ++i) {
+ if (mangled_name_to_index.GetValueAtIndex(i, entry.value)) {
+entry.cstring = mangled_name_to_index.GetCStringAtIndex(i);
+if (class_contexts.find(symbol_contexts[entry.value]) ==
+class_contexts.end()) {
+ // If we got here, we have something that had a context (was inside
+ // a namespace or class) yet we don't know if the entry
+ m_basename_to_index.Append(entry);
}
}
}
+
m_name_to_index.Sort();
m_name_to_index.SizeToFit();
m_selector_to_index.Sort();
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -310,22 +310,16 @@
m_method_to_index.Append(entry);
} else {
if (const_context && const_context[0]) {
- if (class_contexts.find(const_context) !=
- class_contexts.end()) {
-// The current decl context is in our "class_contexts" which
-// means
-// this is a method on a class
-m_method_to_index.Append(entry);
- } else {
-// We don't know if this is a function basename or a method,
-// so put it into a temporary collection so once we are done
-// we can look in class_contexts to see if each entry is a
-// class
-// or just a function an
[Lldb-commits] [PATCH] D32626: Make the ELF symbol demangling loop order independent
scott.smith added a comment.
I welcome any suggestions on how to update the comments near the code I
touched. I can make the code functionally the same, but it doesn't mean I know
why it's doing what it's doing :-)
Comment at: source/Symbol/Symtab.cpp:382
- entry.cstring = mangled_name_to_index.GetCStringAtIndex(i);
- if (symbol_contexts[entry.value] &&
- class_contexts.find(symbol_contexts[entry.value]) !=
Note the old code checked that symbol_contexts[] (aka const_context above) is
"true." However, because of the code above, we would never get here unless it
is true. So I removed the check.
Comment at: source/Symbol/Symtab.cpp:385
- class_contexts.end()) {
-m_method_to_index.Append(entry);
- } else {
Note that adding to m_method_to_index happened in both code paths in the old
code, so I moved it up to the main loop.
Repository:
rL LLVM
https://reviews.llvm.org/D32626
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32568: Protect Proces::GetMemoryRegionInfo and ::GetFileLoadAddress with a lock
scott.smith abandoned this revision. scott.smith added a comment. Further testing shows this is not necessary. I'll abandon it. Repository: rL LLVM https://reviews.llvm.org/D32568 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32626: Make the symbol demangling loop order independent
scott.smith abandoned this revision. scott.smith added a comment. Turns out I'm planning on making more drastic changes to this loop, so there's no point in reviewing this step. Repository: rL LLVM https://reviews.llvm.org/D32626 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32503: Remove unused code related to CPlusPlusLanguage::FindEquivalentNames
scott.smith added a comment. In https://reviews.llvm.org/D32503#738274, @scott.smith wrote: > Can someone please commit this for me? I don't have access. Thank you! Ping - please commit for me! Repository: rL LLVM https://reviews.llvm.org/D32503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key
scott.smith added a comment. In https://reviews.llvm.org/D32316#739699, @scott.smith wrote: > Can I get a re-review on this? Thanks. Ping - please rereview! Repository: rL LLVM https://reviews.llvm.org/D32316 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key
scott.smith added a comment. In https://reviews.llvm.org/D32316#742286, @clayborg wrote: > We can iterate on this. Thank you! Also, can you please push this? I don't have commit access. Repository: rL LLVM https://reviews.llvm.org/D32316 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32708: Check for lack of C++ context first when demangling
scott.smith added a comment. FYI, moving this code changes how two symbols (out of 2M+) are handled on a test program I have. I noticed this because when I changed the loop to set it up for parallelization, I effectively deferred all handling of C++ names to later. It simplifies the code to handle !const_context situations first, and then handle the rest. At a high level I assume that should be equivalent, but it actually changed two symbols. I send this review out so you can either: 1. Tell me it's wrong to move the if statement, because of some situation I don't understand, or 2. Tell me this is correct, and the two symbols that are handled differently actually should be handled differently, or 3. Tell me this is correct, and the two symbols that are handled differently are due to a bug somewhere else. FWIW the two symbols demangle to roughly "thing". lots_of_stuff includes lambdas, vectors, and some other things. It wouldn't surprise me if I stumbled across a bug in the mangler or demangler (there are lots of repeated sections, so it could be a bug in back references). Repository: rL LLVM https://reviews.llvm.org/D32708 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32708: Check for lack of C++ context first when demangling
scott.smith added a comment. Can someone please commit this? Thank you! Repository: rL LLVM https://reviews.llvm.org/D32708 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32708: Check for lack of C++ context first when demangling
scott.smith added a comment. In https://reviews.llvm.org/D32708#743161, @labath wrote: > I am having trouble applying this. Do you need to rebase or something? It was based on another commit that you committed for me, but committed after trying to commit this one. It should apply now that you committed https://reviews.llvm.org/D32316. Repository: rL LLVM https://reviews.llvm.org/D32708 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith created this revision.
Many parallel tasks just want to iterate over all the possible numbers from 0
to N-1. Rather than enqueue N work items, instead just "map" the function
across the requested integer space.
Repository:
rL LLVM
https://reviews.llvm.org/D32757
Files:
include/lldb/Utility/TaskPool.h
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Utility/TaskPool.cpp
unittests/Utility/TaskPoolTest.cpp
Index: unittests/Utility/TaskPoolTest.cpp
===
--- unittests/Utility/TaskPoolTest.cpp
+++ unittests/Utility/TaskPoolTest.cpp
@@ -52,3 +52,15 @@
ASSERT_EQ(4, count);
}
+
+TEST(TaskPoolTest, TaskMap) {
+ int data[4];
+ auto fn = [&data](int x) { data[x] = x * x; };
+
+ TaskMap(4, 1, fn);
+
+ ASSERT_EQ(data[0] == 0);
+ ASSERT_EQ(data[1] == 1);
+ ASSERT_EQ(data[2] == 4);
+ ASSERT_EQ(data[3] == 9);
+}
Index: source/Utility/TaskPool.cpp
===
--- source/Utility/TaskPool.cpp
+++ source/Utility/TaskPool.cpp
@@ -73,3 +73,29 @@
f();
}
}
+
+void TaskMap(size_t size, size_t batch, std::function const & func)
+{
+ std::atomic idx{0};
+ size_t est_batches = std::min((size + batch - 1) / batch,
+std::thread::hardware_concurrency());
+
+ auto wrapper = [&idx, size, batch, &func]()
+ {
+while (true) {
+ size_t batch_start = idx.fetch_add(batch);
+ if (batch_start >= size)
+break;
+ size_t batch_end = std::min(batch_start + batch, size);
+ for (size_t i = batch_start; i < batch_end; i++)
+func(i);
+}
+ };
+
+ std::vector> futures;
+ futures.reserve(est_batches);
+ for (size_t i = 0; i < est_batches; i++)
+futures.push_back(TaskPool::AddTask(wrapper));
+ for (size_t i = 0; i < est_batches; i++)
+futures[i].wait();
+}
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1946,7 +1946,8 @@
std::vector type_index(num_compile_units);
std::vector namespace_index(num_compile_units);
-std::vector clear_cu_dies(num_compile_units, false);
+// std::vector might be implemented using bit test-and-set, so use uint8_t instead.
+std::vector clear_cu_dies(num_compile_units, false);
auto parser_fn = [debug_info, &function_basename_index,
&function_fullname_index, &function_method_index,
&function_selector_index, &objc_class_selectors_index,
@@ -1963,22 +1964,18 @@
return cu_idx;
};
-auto extract_fn = [debug_info](uint32_t cu_idx) {
+auto extract_fn = [debug_info, &clear_cu_dies](uint32_t cu_idx) {
DWARFCompileUnit *dwarf_cu = debug_info->GetCompileUnitAtIndex(cu_idx);
if (dwarf_cu) {
// dwarf_cu->ExtractDIEsIfNeeded(false) will return zero if the
// DIEs for a compile unit have already been parsed.
-return std::make_pair(cu_idx, dwarf_cu->ExtractDIEsIfNeeded(false) > 1);
+if (dwarf_cu->ExtractDIEsIfNeeded(false) > 1)
+ clear_cu_dies[cu_idx] = true;
}
- return std::make_pair(cu_idx, false);
};
// Create a task runner that extracts dies for each DWARF compile unit in a
// separate thread
-TaskRunner> task_runner_extract;
-for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
- task_runner_extract.AddTask(extract_fn, cu_idx);
-
//--
// First figure out which compile units didn't have their DIEs already
// parsed and remember this. If no DIEs were parsed prior to this index
@@ -1988,30 +1985,15 @@
// a DIE in one compile unit refers to another and the indexes accesses
// those DIEs.
//--
-while (true) {
- auto f = task_runner_extract.WaitForNextCompletedTask();
- if (!f.valid())
-break;
- unsigned cu_idx;
- bool clear;
- std::tie(cu_idx, clear) = f.get();
- clear_cu_dies[cu_idx] = clear;
-}
+TaskMap(num_compile_units, 1, extract_fn);
// Now create a task runner that can index each DWARF compile unit in a
// separate
// thread so we can index quickly.
-TaskRunner task_runner;
-for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
- task_runner.AddTask(parser_fn, cu_idx);
-
-while (true) {
- std::future f = task_runner.WaitForNextCompletedTask();
- if (!f.valid())
-break;
- uint32_t cu_idx = f.get();
+TaskMap(num_compile_units, 1, parser_fn);
+for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx) {
m_function_basename_index.Append(function_basename_index[cu_idx]);
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith added a comment. IMO, this is a simpler interface than TaskRunner. Also, after this, there are no users of TaskRunner left. Should I just delete them? I did this change to help parallel symbol demangling (to come in a separate patch). Rather than have the symbol demangler use batching, etc, I thought it should be a higher level function. Repository: rL LLVM https://reviews.llvm.org/D32757 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith added a comment. In https://reviews.llvm.org/D32757#743657, @clayborg wrote: > In https://reviews.llvm.org/D32757#743567, @scott.smith wrote: > > > IMO, this is a simpler interface than TaskRunner. Also, after this, there > > are no users of TaskRunner left. Should I just delete them? > > > I think you might not have understood TaskRunner's real benefits. It is smart > in the way it runs things: it lets you run N things and get each item **as > soon as it completes**. The TaskMap will serialize everything. So if you have > 100 items to do and item 0 takes 200 seconds to complete, and items 1 - 99 > take 1ms to complete, you will need to wait for task 0 to complete before you > can start parsing the data. This will slow down the DWARF parser if you > switch over to this. TaskRunner should not be deleted as it has a very > specific purpose. Your TaskMap works fine for one case, but not in the other. That may provide a benefit on machines with a few cores, but doesn't really help when you have 40+ cores. Granted, the average laptop has 4 cores/8 hyperthreads. >> I did this change to help parallel symbol demangling (to come in a separate >> patch). Rather than have the symbol demangler use batching, etc, I thought >> it should be a higher level function. > > Make sure this is a win before switching demangling over to using threads. I > tried to improve performance on demangling before and I got worse performance > on MacOS when we tried it because all of the malloc contention and threading > overhead. It is on my machine, but maybe not on other machines. That would be unfortunate. I'm guessing I shouldn't add go ahead and add jemalloc/tcmalloc to work around poor a MacOS malloc? haha. Repository: rL LLVM https://reviews.llvm.org/D32757 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith added a comment.
I can make more measurements on this.
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1994
-TaskRunner task_runner;
-for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
- task_runner.AddTask(parser_fn, cu_idx);
-
-while (true) {
- std::future f = task_runner.WaitForNextCompletedTask();
- if (!f.valid())
-break;
- uint32_t cu_idx = f.get();
+TaskMap(num_compile_units, 1, parser_fn);
clayborg wrote:
> Note that we lost performance here. The old code would run:
>
> ```
> while (true) {
> std::future f = task_runner.WaitForNextCompletedTask();
> // Do expensive work as soon as possible with any random task that
> completes as soon as it completes.
> ```
>
> Your current code says "wait to do all expensive work until I complete
> everything and then do all of the expensive work".
>
>
>
>
The following loop is not expensive, it's simple vector concatenation of fairly
simple types. The actual slow work is Finalize(), which calls Sort().
m_function_basename_index is of type NameDIE, which has a UniqueCStringMap
member, which declared collection as std::vector.
Though arguably the Append should be pushed down into the RunTasks below.
Comment at: source/Utility/TaskPool.cpp:100
+ for (size_t i = 0; i < est_batches; i++)
+futures[i].wait();
+}
clayborg wrote:
> TaskRunner is smart in the way it runs things: it lets you run N things and
> get each item as it completes. This implementation, if read it correctly,
> will serialize everything. So if you have 100 items to do and item at index 0
> takes 200 seconds to complete, and items 1 - 99 take 1ms to complete, you
> will need to wait for task 0 to complete before you can start parsing the
> data. This will slow down the DWARF parser if you switch over to this.
If items 1-99 complete quickly, there isn't much advantage to handling their
output before handling the output of item 0, since item 0 is likely to produce
more output than 1-99 combined.
Repository:
rL LLVM
https://reviews.llvm.org/D32757
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith marked 6 inline comments as done.
scott.smith added a comment.
IMO the vector append issue doesn't matter, because the very next thing we do
is sort. Sorting is more expensive than appending, so the append is noise.
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1994
-TaskRunner task_runner;
-for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
- task_runner.AddTask(parser_fn, cu_idx);
-
-while (true) {
- std::future f = task_runner.WaitForNextCompletedTask();
- if (!f.valid())
-break;
- uint32_t cu_idx = f.get();
+TaskMap(num_compile_units, 1, parser_fn);
scott.smith wrote:
> clayborg wrote:
> > Note that we lost performance here. The old code would run:
> >
> > ```
> > while (true) {
> > std::future f = task_runner.WaitForNextCompletedTask();
> > // Do expensive work as soon as possible with any random task that
> > completes as soon as it completes.
> > ```
> >
> > Your current code says "wait to do all expensive work until I complete
> > everything and then do all of the expensive work".
> >
> >
> >
> >
> The following loop is not expensive, it's simple vector concatenation of
> fairly simple types. The actual slow work is Finalize(), which calls Sort().
>
> m_function_basename_index is of type NameDIE, which has a UniqueCStringMap
> member, which declared collection as std::vector.
>
> Though arguably the Append should be pushed down into the RunTasks below.
>
This takes <0.25s (out of a total of 15 seconds of runtime) when timing lldb
starting lldb (RelWithDebInfo build). That's for an aggregate of 14M+ names
being added to the vectors. IMO that should not block this change.
I also moved the append into RunTasks, just because we already have those
subtasks.
Comment at: source/Utility/TaskPool.cpp:77
+
+void TaskMap(size_t size, size_t batch, std::function const &
func)
+{
clayborg wrote:
> Is this named correctly? Maybe SerializedTaskMap? Or BatchedTaskMap?
TaskMapOverInt?
Repository:
rL LLVM
https://reviews.llvm.org/D32757
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith updated this revision to Diff 97494.
scott.smith marked 2 inline comments as done.
scott.smith added a comment.
change name to TaskRunOverint
remove TaskRunner
Repository:
rL LLVM
https://reviews.llvm.org/D32757
Files:
include/lldb/Utility/TaskPool.h
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Utility/TaskPool.cpp
unittests/Utility/TaskPoolTest.cpp
Index: unittests/Utility/TaskPoolTest.cpp
===
--- unittests/Utility/TaskPoolTest.cpp
+++ unittests/Utility/TaskPoolTest.cpp
@@ -30,25 +30,14 @@
ASSERT_EQ(17, r[3]);
}
-TEST(TaskPoolTest, TaskRunner) {
- auto fn = [](int x) { return std::make_pair(x, x * x); };
-
- TaskRunner> tr;
- tr.AddTask(fn, 1);
- tr.AddTask(fn, 2);
- tr.AddTask(fn, 3);
- tr.AddTask(fn, 4);
-
- int count = 0;
- while (true) {
-auto f = tr.WaitForNextCompletedTask();
-if (!f.valid())
- break;
-
-++count;
-std::pair v = f.get();
-ASSERT_EQ(v.first * v.first, v.second);
- }
-
- ASSERT_EQ(4, count);
+TEST(TaskPoolTest, TaskMap) {
+ int data[4];
+ auto fn = [&data](int x) { data[x] = x * x; };
+
+ TaskMap(4, 1, fn);
+
+ ASSERT_EQ(data[0] == 0);
+ ASSERT_EQ(data[1] == 1);
+ ASSERT_EQ(data[2] == 4);
+ ASSERT_EQ(data[3] == 9);
}
Index: source/Utility/TaskPool.cpp
===
--- source/Utility/TaskPool.cpp
+++ source/Utility/TaskPool.cpp
@@ -73,3 +73,30 @@
f();
}
}
+
+void TaskMapOverInt(size_t size, size_t batch,
+std::function const & func)
+{
+ std::atomic idx{0};
+ size_t num_workers = std::min((size + batch - 1) / batch,
+std::thread::hardware_concurrency());
+
+ auto wrapper = [&idx, size, batch, &func]()
+ {
+while (true) {
+ size_t batch_start = idx.fetch_add(batch);
+ if (batch_start >= size)
+break;
+ size_t batch_end = std::min(batch_start + batch, size);
+ for (size_t i = batch_start; i < batch_end; i++)
+func(i);
+}
+ };
+
+ std::vector> futures;
+ futures.reserve(num_workers);
+ for (size_t i = 0; i < num_workers; i++)
+futures.push_back(TaskPool::AddTask(wrapper));
+ for (size_t i = 0; i < num_workers; i++)
+futures[i].wait();
+}
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1946,7 +1946,8 @@
std::vector type_index(num_compile_units);
std::vector namespace_index(num_compile_units);
-std::vector clear_cu_dies(num_compile_units, false);
+// std::vector might be implemented using bit test-and-set, so use uint8_t instead.
+std::vector clear_cu_dies(num_compile_units, false);
auto parser_fn = [debug_info, &function_basename_index,
&function_fullname_index, &function_method_index,
&function_selector_index, &objc_class_selectors_index,
@@ -1963,22 +1964,18 @@
return cu_idx;
};
-auto extract_fn = [debug_info](uint32_t cu_idx) {
+auto extract_fn = [debug_info, &clear_cu_dies](uint32_t cu_idx) {
DWARFCompileUnit *dwarf_cu = debug_info->GetCompileUnitAtIndex(cu_idx);
if (dwarf_cu) {
// dwarf_cu->ExtractDIEsIfNeeded(false) will return zero if the
// DIEs for a compile unit have already been parsed.
-return std::make_pair(cu_idx, dwarf_cu->ExtractDIEsIfNeeded(false) > 1);
+if (dwarf_cu->ExtractDIEsIfNeeded(false) > 1)
+ clear_cu_dies[cu_idx] = true;
}
- return std::make_pair(cu_idx, false);
};
// Create a task runner that extracts dies for each DWARF compile unit in a
// separate thread
-TaskRunner> task_runner_extract;
-for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
- task_runner_extract.AddTask(extract_fn, cu_idx);
-
//--
// First figure out which compile units didn't have their DIEs already
// parsed and remember this. If no DIEs were parsed prior to this index
@@ -1988,48 +1985,33 @@
// a DIE in one compile unit refers to another and the indexes accesses
// those DIEs.
//--
-while (true) {
- auto f = task_runner_extract.WaitForNextCompletedTask();
- if (!f.valid())
-break;
- unsigned cu_idx;
- bool clear;
- std::tie(cu_idx, clear) = f.get();
- clear_cu_dies[cu_idx] = clear;
-}
+TaskMapOverInt(num_compile_units, 1, extract_fn);
// Now create a task runner that can index each DWARF compile unit in a
// separate
// thread so we can index quickly.
-TaskRunner task_runner;
-for (uint32_t cu_idx = 0; cu_idx < num_compile_
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith added a comment. In https://reviews.llvm.org/D32757#743796, @zturner wrote: > s/instead of LLVM/instead of LLDB/ I hear you, but IMO it's not ready for that yet. 1. It would depend on ThreadPool, but 2. LLDB hasn't switched to ThreadPool yet, because 3. I want to figure out how to incorporate tasks enqueuing tasks first. I don't want to commit a monolithic patch with all my changes (and I haven't developed them all yet), so instead I submit incremental improvements. Repository: rL LLVM https://reviews.llvm.org/D32757 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith added a comment. In https://reviews.llvm.org/D32757#743874, @clayborg wrote: > So I don't see where you moved all of the .Append functions. And if you did > your timings with them not being in there then your timings are off. finalize_fn calls Append on each source first, then calls Finalize. It might be hard to see because it's now a lambda that takes the src vector and dst NameToDIE as parameters. Repository: rL LLVM https://reviews.llvm.org/D32757 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32597: Initiate loading of shared libraries in parallel
scott.smith updated this revision to Diff 97695.
scott.smith added a comment.
update to use a private llvm::ThreadPool. I chose this over a 2nd global
"TaskPool" because if the threads are going to be short lived, there isn't much
point in having a global pool rather than a short-lived instantiated one.
Repository:
rL LLVM
https://reviews.llvm.org/D32597
Files:
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
Index: source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
===
--- source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
+++ source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
@@ -66,6 +66,10 @@
uint32_t GetPluginVersion() override;
protected:
+ /// Mutex to protect various global variables during parallel shared library
+ /// loading.
+ std::recursive_mutex m_mutex;
+
/// Runtime linker rendezvous structure.
DYLDRendezvous m_rendezvous;
Index: source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
===
--- source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -27,6 +27,7 @@
#include "lldb/Target/Thread.h"
#include "lldb/Target/ThreadPlanRunToAddress.h"
#include "lldb/Utility/Log.h"
+#include "llvm/Support/ThreadPool.h"
// C++ Includes
// C Includes
@@ -195,6 +196,7 @@
}
void DynamicLoaderPOSIXDYLD::DidLaunch() {
+ std::lock_guard guard(m_mutex);
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
if (log)
log->Printf("DynamicLoaderPOSIXDYLD::%s()", __FUNCTION__);
@@ -228,17 +230,20 @@
addr_t link_map_addr,
addr_t base_addr,
bool base_addr_is_offset) {
+ std::lock_guard guard(m_mutex);
m_loaded_modules[module] = link_map_addr;
UpdateLoadedSectionsCommon(module, base_addr, base_addr_is_offset);
}
void DynamicLoaderPOSIXDYLD::UnloadSections(const ModuleSP module) {
+ std::lock_guard guard(m_mutex);
m_loaded_modules.erase(module);
UnloadSectionsCommon(module);
}
void DynamicLoaderPOSIXDYLD::ProbeEntry() {
+ std::lock_guard guard(m_mutex);
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
const addr_t entry = GetEntryPoint();
@@ -329,6 +334,7 @@
}
void DynamicLoaderPOSIXDYLD::SetRendezvousBreakpoint() {
+ std::lock_guard guard(m_mutex);
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
addr_t break_addr = m_rendezvous.GetBreakAddress();
@@ -372,6 +378,7 @@
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
DynamicLoaderPOSIXDYLD *const dyld_instance =
static_cast(baton);
+ std::lock_guard guard(dyld_instance->m_mutex);
if (log)
log->Printf("DynamicLoaderPOSIXDYLD::%s called for pid %" PRIu64,
__FUNCTION__,
@@ -393,6 +400,7 @@
}
void DynamicLoaderPOSIXDYLD::RefreshModules() {
+ std::lock_guard guard(m_mutex);
if (!m_rendezvous.Resolve())
return;
@@ -437,6 +445,7 @@
ThreadPlanSP
DynamicLoaderPOSIXDYLD::GetStepThroughTrampolinePlan(Thread &thread,
bool stop) {
+ std::lock_guard guard(m_mutex);
ThreadPlanSP thread_plan_sp;
StackFrame *frame = thread.GetStackFrameAtIndex(0).get();
@@ -514,14 +523,33 @@
std::vector module_names;
for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I)
module_names.push_back(I->file_spec);
+ size_t num_to_load = module_names.size();
m_process->PrefetchModuleSpecs(
module_names, m_process->GetTarget().GetArchitecture().GetTriple());
- for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I) {
-ModuleSP module_sp =
-LoadModuleAtAddress(I->file_spec, I->link_addr, I->base_addr, true);
-if (module_sp.get()) {
- module_list.Append(module_sp);
+ struct loader {
+DYLDRendezvous::iterator I;
+ModuleSP m;
+std::shared_future f;
+ };
+ std::vector loaders(num_to_load);
+ llvm::ThreadPool load_pool(
+std::min(num_to_load, std::thread::hardware_concurrency()));
+ auto loader_fn = [this](loader * l)
+ {
+l->m = LoadModuleAtAddress(
+ l->I->file_spec, l->I->link_addr, l->I->base_addr, true);
+ };
+
+ loader * l = loaders.data();
+ for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I, ++l) {
+l->I = I;
+l->f = load_pool.async(loader_fn, l);
+ }
+ for (auto & l : loaders) {
+l.f.wait();
+if (l.m.get()) {
+ module_list.Append(l.m);
} else {
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
if (log)
@@ -535,6 +563,7 @@
}
addr_t DynamicLoaderPOSIXDYLD::ComputeLoadOff
[Lldb-commits] [PATCH] D32820: Parallelize demangling
scott.smith created this revision.
Use TaskMapOverInt to demangle the symbols in parallel. Defer categorization
of C++ symbols until later, when it can be determined what contexts are
definitely classes, and what might not be.
Repository:
rL LLVM
https://reviews.llvm.org/D32820
Files:
include/lldb/Utility/ConstString.h
source/Symbol/Symtab.cpp
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -22,6 +22,7 @@
#include "lldb/Symbol/Symtab.h"
#include "lldb/Utility/RegularExpression.h"
#include "lldb/Utility/Stream.h"
+#include "lldb/Utility/TaskPool.h"
using namespace lldb;
using namespace lldb_private;
@@ -243,42 +244,44 @@
m_name_to_index.Reserve(actual_count);
#endif
-NameToIndexMap::Entry entry;
-
-// The "const char *" in "class_contexts" must come from a
-// ConstString::GetCString()
-std::set class_contexts;
-UniqueCStringMap mangled_name_to_index;
-std::vector symbol_contexts(num_symbols, nullptr);
+struct demangle_state {
+ ConstString name_to_index[5];
+ ConstString selector_to_index[1];
+ ConstString const_context;
+ ConstString cxx_basename;
+ bool is_definitely_class_context = false;
+};
+std::vector states(num_symbols);
-for (entry.value = 0; entry.value < num_symbols; ++entry.value) {
- const Symbol *symbol = &m_symbols[entry.value];
+auto symbol_fn = [&states, this](size_t value) {
+ const Symbol *symbol = &m_symbols[value];
// Don't let trampolines get into the lookup by name map
// If we ever need the trampoline symbols to be searchable by name
// we can remove this and then possibly add a new bool to any of the
// Symtab functions that lookup symbols by name to indicate if they
// want trampolines.
if (symbol->IsTrampoline())
-continue;
+return;
+ demangle_state &state = states[value];
const Mangled &mangled = symbol->GetMangled();
- entry.cstring = mangled.GetMangledName();
- if (entry.cstring) {
-m_name_to_index.Append(entry);
+ ConstString cstring = mangled.GetMangledName();
+ if (cstring) {
+state.name_to_index[0] = cstring;
if (symbol->ContainsLinkerAnnotations()) {
// If the symbol has linker annotations, also add the version without
// the annotations.
- entry.cstring = ConstString(m_objfile->StripLinkerSymbolAnnotations(
-entry.cstring.GetStringRef()));
- m_name_to_index.Append(entry);
+ cstring = ConstString(m_objfile->StripLinkerSymbolAnnotations(
+cstring.GetStringRef()));
+ state.name_to_index[1] = cstring;
}
const SymbolType symbol_type = symbol->GetType();
if (symbol_type == eSymbolTypeCode ||
symbol_type == eSymbolTypeResolver) {
- llvm::StringRef entry_ref(entry.cstring.GetStringRef());
+ llvm::StringRef entry_ref(cstring.GetStringRef());
if (entry_ref[0] == '_' && entry_ref[1] == 'Z' &&
(entry_ref[2] != 'T' && // avoid virtual table, VTT structure,
// typeinfo structure, and typeinfo
@@ -290,103 +293,92 @@
{
CPlusPlusLanguage::MethodName cxx_method(
mangled.GetDemangledName(lldb::eLanguageTypeC_plus_plus));
-entry.cstring = ConstString(cxx_method.GetBasename());
-if (entry.cstring) {
+cstring = ConstString(cxx_method.GetBasename());
+if (cstring) {
// ConstString objects permanently store the string in the pool so
// calling
// GetCString() on the value gets us a const char * that will
// never go away
- const char *const_context =
- ConstString(cxx_method.GetContext()).GetCString();
-
- if (!const_context || const_context[0] == 0) {
-// No context for this function so this has to be a basename
-m_basename_to_index.Append(entry);
-// If there is no context (no namespaces or class scopes that
-// come before the function name) then this also could be a
-// fullname.
-m_name_to_index.Append(entry);
- } else {
-entry_ref = entry.cstring.GetStringRef();
-if (entry_ref[0] == '~' ||
-!cxx_method.GetQualifiers().empty()) {
- // The first character of the demangled basename is '~' which
- // means we have a class destructor. We can use this information
- // to help us know what is a class and what isn't.
- if (class_contexts.find(const_context) == class_contexts.end())
-
[Lldb-commits] [PATCH] D32820: Parallelize demangling
scott.smith added a comment. This commit uses TaskMapOverInt from change https://reviews.llvm.org/D32757, which hasn't landed yet. Repository: rL LLVM https://reviews.llvm.org/D32820 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32823: Remove an expensive lock from Timer
scott.smith created this revision.
The Timer destructor would grab a global mutex in order to update execution
time. Add a class to define a category once, statically; the class adds itself
to an atomic singly linked list, and thus subsequent updates only need to use
an atomic rather than grab a lock and perform a hashtable lookup.
Repository:
rL LLVM
https://reviews.llvm.org/D32823
Files:
include/lldb/Core/Timer.h
source/API/SystemInitializerFull.cpp
source/Commands/CommandObjectTarget.cpp
source/Core/Disassembler.cpp
source/Core/Mangled.cpp
source/Core/Module.cpp
source/Core/Timer.cpp
source/Host/common/Symbols.cpp
source/Initialization/SystemInitializerCommon.cpp
source/Interpreter/CommandInterpreter.cpp
source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugPubnames.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
source/Symbol/DWARFCallFrameInfo.cpp
source/Symbol/ObjectFile.cpp
source/Symbol/Symtab.cpp
source/Target/Target.cpp
source/Target/TargetList.cpp
Index: source/Target/TargetList.cpp
===
--- source/Target/TargetList.cpp
+++ source/Target/TargetList.cpp
@@ -325,7 +325,8 @@
lldb::PlatformSP &platform_sp,
lldb::TargetSP &target_sp,
bool is_dummy_target) {
- Timer scoped_timer(LLVM_PRETTY_FUNCTION,
+ static TimerCategory func_cat(LLVM_PRETTY_FUNCTION);
+ Timer scoped_timer(func_cat,
"TargetList::CreateTarget (file = '%s', arch = '%s')",
user_exe_path.str().c_str(),
specified_arch.GetArchitectureName());
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1235,7 +1235,8 @@
ClearModules(false);
if (executable_sp) {
-Timer scoped_timer(LLVM_PRETTY_FUNCTION,
+static TimerCategory func_cat(LLVM_PRETTY_FUNCTION);
+Timer scoped_timer(func_cat,
"Target::SetExecutableModule (executable = '%s')",
executable_sp->GetFileSpec().GetPath().c_str());
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -220,7 +220,8 @@
// Protected function, no need to lock mutex...
if (!m_name_indexes_computed) {
m_name_indexes_computed = true;
-Timer scoped_timer(LLVM_PRETTY_FUNCTION, "%s", LLVM_PRETTY_FUNCTION);
+static TimerCategory func_cat(LLVM_PRETTY_FUNCTION);
+Timer scoped_timer(func_cat, "%s", LLVM_PRETTY_FUNCTION);
// Create the name index vector to be able to quickly search by name
const size_t num_symbols = m_symbols.size();
#if 1
@@ -433,7 +434,8 @@
bool add_demangled, bool add_mangled,
NameToIndexMap &name_to_index_map) const {
if (add_demangled || add_mangled) {
-Timer scoped_timer(LLVM_PRETTY_FUNCTION, "%s", LLVM_PRETTY_FUNCTION);
+static TimerCategory func_cat(LLVM_PRETTY_FUNCTION);
+Timer scoped_timer(func_cat, "%s", LLVM_PRETTY_FUNCTION);
std::lock_guard guard(m_mutex);
// Create the name index vector to be able to quickly search by name
@@ -595,7 +597,8 @@
bool remove_duplicates) const {
std::lock_guard guard(m_mutex);
- Timer scoped_timer(LLVM_PRETTY_FUNCTION, LLVM_PRETTY_FUNCTION);
+ static TimerCategory func_cat(LLVM_PRETTY_FUNCTION);
+ Timer scoped_timer(func_cat, LLVM_PRETTY_FUNCTION);
// No need to sort if we have zero or one items...
if (indexes.size() <= 1)
return;
@@ -621,7 +624,8 @@
std::vector &indexes) {
std::lock_guard guard(m_mutex);
- Timer scoped_timer(LLVM_PRETTY_FUNCTION, "%s", LLVM_PRETTY_FUNCTION);
+ static TimerCategory func_cat(LLVM_PRETTY_FUNCTION);
+ Timer scoped_timer(func_cat, "%s", LLVM_PRETTY_FUNCTION);
if (symbol_name) {
if (!m_name_indexes_computed)
InitNameIndexes();
@@ -637,7 +641,8 @@
std::vector &indexes) {
std::lock_guard guard(m_mutex);
- Timer scoped_timer(LLVM_PRETTY_FUNCTION, "%s", LLVM_PRETTY_FUNC
[Lldb-commits] [PATCH] D32597: Initiate loading of shared libraries in parallel
scott.smith added inline comments.
Comment at:
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:530-560
+ struct loader {
+DYLDRendezvous::iterator I;
+ModuleSP m;
+std::shared_future f;
+ };
+ std::vector loaders(num_to_load);
+ llvm::ThreadPool load_pool(
zturner wrote:
> zturner wrote:
> > I'm still unclear why we're still going down this route of adding a very
> > specialized instance of parallelism to this one place, that is probably
> > going to be less efficient than having a global thread pool (as now you
> > will have to spin up and destroy the entire thread pool every time you go
> > to load shared libraries).
> >
> > So, rather than keep repeating the same thing over and over, I went and
> > looked into this a little bit. It turns out LLD has a large library of
> > parallel algorithms in `lld/Include/lld/Parallel.h`. There is almost
> > nothing specific to LLD in any of these algorithms however, and they should
> > probably be moved to llvm support. Upon doing so, you will be able to
> > write this code as:
> >
> > ```
> > std::vector Modules(m_rendevous.size());
> > llvm::parallel_for_each(make_integer_range(0, m_rendevous.size()), [this,
> > &Modules](int I)
> > {
> > auto &R = m_rendevous[I];
> > Modules[I] = LoadModuleAtAddress(R.file_spec, R.link_addr, R.base_addr,
> > true);
> > });
> > for (const auto &M : Modules)
> >module_list.Append(M);
> > ```
> >
> > Please look into making this change.
> Note that this is, of course, exactly why I've been saying all along that we
> should not even be discussing this here. Had the discussion happened over on
> the LLVM side the first time I asked about this, someone could have mentioned
> this without me having to spend time looking into it.
As long as those routines allow you to recursively call those routines, then
great, go for it.
The spinning up/tearing down of threads is not expensive, at least in my
measured use case.
Repository:
rL LLVM
https://reviews.llvm.org/D32597
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32832: Make ConstString creation and access lockfree
scott.smith created this revision.
Utilize atomics to update linked lists without requiring locks. Reduces the
number of calls to compute the hash by not having to compute it once to
determine the bucket and then again inside a separate hashtable implementation.
Use xxhash for better performance, especially with longer strings. Eliminates
calls to hash when just updating the value by being able to use atomics instead
of locks.
Repository:
rL LLVM
https://reviews.llvm.org/D32832
Files:
source/Utility/ConstString.cpp
Index: source/Utility/ConstString.cpp
===
--- source/Utility/ConstString.cpp
+++ source/Utility/ConstString.cpp
@@ -18,6 +18,7 @@
#include "llvm/Support/FormatProviders.h" // for format_provider
#include "llvm/Support/RWMutex.h"
#include "llvm/Support/Threading.h"
+#include "llvm/Support/xxhash.h"
#include // for min
#include
@@ -31,47 +32,58 @@
class Pool {
public:
- typedef const char *StringPoolValueType;
- typedef llvm::StringMap
- StringPool;
- typedef llvm::StringMapEntry StringPoolEntryType;
+ struct Entry {
+uint64_t hash;
+size_t key_len;
+std::atomic value;
+std::atomic next;
+// string follows
+void setValue(const char * val) {
+ value.store(val, std::memory_order_release);
+}
+const char * cstr() const {
+ return reinterpret_cast(this + 1);
+}
+llvm::StringRef str() const {
+ return llvm::StringRef(cstr(), key_len);
+}
+ };
+
+ Pool() {
+for (auto & a : m_string_pools)
+ a.store(nullptr, std::memory_order_release);
+ }
+
+ typedef std::atomic StringPool;
+ typedef Entry StringPoolEntryType;
static StringPoolEntryType &
GetStringMapEntryFromKeyData(const char *keyData) {
-return StringPoolEntryType::GetStringMapEntryFromKeyData(keyData);
+StringPoolEntryType * p = reinterpret_cast(const_cast(keyData));
+return *(p - 1);
}
static size_t GetConstCStringLength(const char *ccstr) {
if (ccstr != nullptr) {
// Since the entry is read only, and we derive the entry entirely from the
// pointer, we don't need the lock.
const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr);
- return entry.getKey().size();
+ return entry.key_len;
}
return 0;
}
- StringPoolValueType GetMangledCounterpart(const char *ccstr) const {
+ const char * GetMangledCounterpart(const char *ccstr) const {
if (ccstr != nullptr) {
- const uint8_t h = hash(llvm::StringRef(ccstr));
- llvm::sys::SmartScopedReader rlock(m_string_pools[h].m_mutex);
- return GetStringMapEntryFromKeyData(ccstr).getValue();
+ return GetStringMapEntryFromKeyData(ccstr).value;
}
return nullptr;
}
bool SetMangledCounterparts(const char *key_ccstr, const char *value_ccstr) {
if (key_ccstr != nullptr && value_ccstr != nullptr) {
- {
-const uint8_t h = hash(llvm::StringRef(key_ccstr));
-llvm::sys::SmartScopedWriter wlock(m_string_pools[h].m_mutex);
-GetStringMapEntryFromKeyData(key_ccstr).setValue(value_ccstr);
- }
- {
-const uint8_t h = hash(llvm::StringRef(value_ccstr));
-llvm::sys::SmartScopedWriter wlock(m_string_pools[h].m_mutex);
-GetStringMapEntryFromKeyData(value_ccstr).setValue(key_ccstr);
- }
+ GetStringMapEntryFromKeyData(key_ccstr).setValue(value_ccstr);
+ GetStringMapEntryFromKeyData(value_ccstr).setValue(key_ccstr);
return true;
}
return false;
@@ -91,53 +103,50 @@
const char *GetConstCStringWithStringRef(const llvm::StringRef &string_ref) {
if (string_ref.data()) {
- const uint8_t h = hash(string_ref);
-
- {
-llvm::sys::SmartScopedReader rlock(m_string_pools[h].m_mutex);
-auto it = m_string_pools[h].m_string_map.find(string_ref);
-if (it != m_string_pools[h].m_string_map.end())
- return it->getKeyData();
+ const uint64_t h = hash(string_ref);
+ while (true) {
+auto * b = &bucket(h);
+Entry * bref;
+while ((bref = b->load())) {
+ if (bref->hash > h)
+break;
+ if (bref->hash == h) {
+// Hash collision, do a secondary sort on string
+llvm::StringRef bstringref = bref->str();
+if (string_ref == bstringref)
+ return bref->cstr();
+if (bstringref > string_ref)
+ break;
+ }
+ b = &bref->next;
+}
+Entry * new_entry = reinterpret_cast(
+ malloc(sizeof(Entry) + string_ref.size() + 1));
+new_entry->hash = h;
+new_entry->key_len = string_ref.size();
+new_entry->value.store(nullptr, std::memory_order_relaxed);
+new_entry->next.store(bref, std::memory_order_relaxed);
+char * dest = const_cast(new_entry->cstr());
+memcpy(dest, string_ref.data(), string_ref.size());
+
[Lldb-commits] [PATCH] D32832: Make ConstString creation and access lockfree
scott.smith added a comment. Note, I don't expect you guys to want this as is, since the # of buckets can't grow, and thus for very large applications this will behave O(n) instead of O(1). Before I bother to convert this to 1M skip lists (instead of 1M singly linked lists), is this something you're at all interested in? It brings execution time of my test down from ~4.5s to ~3.5s (lldb -b -o 'b main' -o 'run' my_test_program). Repository: rL LLVM https://reviews.llvm.org/D32832 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32832: Make ConstString creation and access lockfree
scott.smith added a comment. In https://reviews.llvm.org/D32832#745361, @clayborg wrote: > Do you have performance numbers here? I am all for it if it improves > performance. If this is faster than llvm::StringMap why not just fix it > there? Seems like everyone could benefit if you fix it in LLVM? A proper lockfree hashtable that allows deletions is complicated and slow for simple cases, IME. ConstString is a special case, because 1. It's big 2. It's insert-only 3. It's highly concurrent I think a proper lock free SkipList would be a better fit for LLVM though, and this code could then build upon it, which would solve it's existing scalability issue. Truth is this isn't a huge deal (ignoring the HashString->xxHash change and assuming tcmalloc, it's saving 1+ seconds, out of 4.5-5ish). It might lead itself to a custom allocator, though, which may make linking with tcmalloc unnecessary (something like BumpPtrAllocator, but threadsafe, or maybe just thread local). Again, a trick you can get away with since this is insert-only. So to break it down: 1. the HashString->xxHash change could be made separately if there was a version of StringMap that took the hash as a parameter, rather than running the hash function itself. That would reduce the # of calls from 2 to 1, and would allow the use of a function that is better suited to lldb (I ran into resistance changing llvm::HashString since clang depends on it, and I couldn't prove it wouldn't hurt. 2. 64-bit hashes for fewer collisions - StringMap uses 32-bit hashes, which makes sense for smaller hash tables. This may or may not matter much. 3. no lock for reading or updating value - GetMangledCounterpart, SetMangledCounterparts, and half of GetConstCStringAndSetMangledCountrpart no longer compute a hash, and don't need a lock. You can actually do this with StringMap by using a struct as the value side that has std::atomic in it 4. much finer granularity - I tried increasing the # of buckets in the old ConstString but could never improve performance (though that was without tcmalloc, maybe it'd work now) 5. lockfree insertion and possibly in the future: 6. BumpPtrAllocator for struct Entry - maybe eliminate the need for tcmalloc (well, assuming the demangler used an arena allocator, something else I gave up rather than trying to convince libcxxabi to change, then having to port that back to llvm). I haven't measured how much each of these changes matter. This change is small enough and in a file that changes infrequently, so we can keep this as a private patch if we have to. Repository: rL LLVM https://reviews.llvm.org/D32832 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32823: Remove an expensive lock from Timer
scott.smith marked 3 inline comments as done.
scott.smith added a comment.
In https://reviews.llvm.org/D32823#745799, @labath wrote:
> Seems reasonable, however: I am not sure who actually uses these timers. I'd
> be tempted to just remove the timers that are causing the contention.
IMO we should either keep the timers and make them fast, or get rid of all
timers. I don't like the idea of training developers to know that timers are
slow, and should be used sparingly. Especially since the fix is relatively
simple.
Though an atomic skip list would be another way to improve performance without
requiring changing each callsite; the downside is the lg-n lookup when tallying
the time.
Comment at: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5421
char *buf = (char *) malloc (ident_command.cmdsize);
-if (buf != nullptr
+if (buf != nullptr
&& m_data.CopyData (offset, ident_command.cmdsize, buf) ==
ident_command.cmdsize) {
labath wrote:
> I am not sure how you format your changes, but you should make sure you
> format only the lines you've touched, and not the whole files.
> git-clang-format
>
> will do that for you -- when you set it up, you just run `git clang-format
> HEAD^` and it will format your last patch).
Yeah that'll be helpful. I have default .emacs settings for work that don't
necessarily apply well to the llvm codebase, this tool will make it easier to
fix up.
Repository:
rL LLVM
https://reviews.llvm.org/D32823
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32823: Remove an expensive lock from Timer
scott.smith updated this revision to Diff 97880.
scott.smith marked an inline comment as done.
scott.smith added a comment.
updated per review comments.
added a test to fix the Jurassic Park problem.
Repository:
rL LLVM
https://reviews.llvm.org/D32823
Files:
include/lldb/Core/Timer.h
source/API/SystemInitializerFull.cpp
source/Commands/CommandObjectTarget.cpp
source/Core/Disassembler.cpp
source/Core/Mangled.cpp
source/Core/Module.cpp
source/Core/Timer.cpp
source/Host/common/Symbols.cpp
source/Initialization/SystemInitializerCommon.cpp
source/Interpreter/CommandInterpreter.cpp
source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugPubnames.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
source/Symbol/DWARFCallFrameInfo.cpp
source/Symbol/ObjectFile.cpp
source/Symbol/Symtab.cpp
source/Target/Target.cpp
source/Target/TargetList.cpp
unittests/Core/TimerTest.cpp
Index: unittests/Core/TimerTest.cpp
===
--- unittests/Core/TimerTest.cpp
+++ unittests/Core/TimerTest.cpp
@@ -18,7 +18,8 @@
TEST(TimerTest, CategoryTimes) {
Timer::ResetCategoryTimes();
{
-Timer t("CAT1", "");
+static Timer::Category tcat("CAT1");
+Timer t(tcat, "");
std::this_thread::sleep_for(std::chrono::milliseconds(10));
}
StreamString ss;
@@ -32,25 +33,32 @@
TEST(TimerTest, CategoryTimesNested) {
Timer::ResetCategoryTimes();
{
-Timer t1("CAT1", "");
+static Timer::Category tcat1a("CAT1");
+Timer t1(tcat1a, "");
std::this_thread::sleep_for(std::chrono::milliseconds(10));
-Timer t2("CAT1", "");
+// Explicitly testing the same category as above.
+static Timer::Category tcat1b("CAT1");
+Timer t2(tcat1b, "");
std::this_thread::sleep_for(std::chrono::milliseconds(10));
}
StreamString ss;
Timer::DumpCategoryTimes(&ss);
double seconds;
+ // It should only appear once.
+ ASSERT_EQ(nullptr, strstr(strstr(ss.GetData(), "CAT1") + 1, "CAT1"));
ASSERT_EQ(1, sscanf(ss.GetData(), "%lf sec for CAT1", &seconds));
EXPECT_LT(0.002, seconds);
EXPECT_GT(0.2, seconds);
}
TEST(TimerTest, CategoryTimes2) {
Timer::ResetCategoryTimes();
{
-Timer t1("CAT1", "");
+static Timer::Category tcat1("CAT1");
+Timer t1(tcat1, "");
std::this_thread::sleep_for(std::chrono::milliseconds(100));
-Timer t2("CAT2", "");
+static Timer::Category tcat2("CAT2");
+Timer t2(tcat2, "");
std::this_thread::sleep_for(std::chrono::milliseconds(10));
}
StreamString ss;
Index: source/Target/TargetList.cpp
===
--- source/Target/TargetList.cpp
+++ source/Target/TargetList.cpp
@@ -325,10 +325,10 @@
lldb::PlatformSP &platform_sp,
lldb::TargetSP &target_sp,
bool is_dummy_target) {
- Timer scoped_timer(LLVM_PRETTY_FUNCTION,
- "TargetList::CreateTarget (file = '%s', arch = '%s')",
- user_exe_path.str().c_str(),
- specified_arch.GetArchitectureName());
+ static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
+ Timer scoped_timer(
+ func_cat, "TargetList::CreateTarget (file = '%s', arch = '%s')",
+ user_exe_path.str().c_str(), specified_arch.GetArchitectureName());
Error error;
ArchSpec arch(specified_arch);
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1235,7 +1235,8 @@
ClearModules(false);
if (executable_sp) {
-Timer scoped_timer(LLVM_PRETTY_FUNCTION,
+static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
+Timer scoped_timer(func_cat,
"Target::SetExecutableModule (executable = '%s')",
executable_sp->GetFileSpec().GetPath().c_str());
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -220,7 +220,8 @@
// Protected function, no need to lock mutex...
if (!m_name_indexes_computed) {
m_name_indexes_computed = true;
-Timer scoped_timer(LLVM_PRETTY_FUNCTION, "%s", LLVM_PRETTY_FUNCTION);
+static Timer::Categ
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith added a comment. In https://reviews.llvm.org/D32757#743793, @zturner wrote: > Not to sound like a broken record, but please try to put this in LLVM instead > of LLVM. I suggested a convenient function signature earlier. @zturner ok to commit? TaskMapOverInt(x, y, fn) maps directly to parallel_for(0, x, fn). Rather than rebundle the change you have for lldb, only for it to be deleted once you get it into llvm, can we just commit this as a stopgap? It is a step in the right direction as it removes TaskRunner and puts us on an API more likely to end up in LLVM. Repository: rL LLVM https://reviews.llvm.org/D32757 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith updated this revision to Diff 97907.
scott.smith added a comment.
1. Change the API to more closely match parallel_for (begin, end, fn) instead
of (end, batch, fn).
2. Fix unit test. I didn't realize I had to run check-lldb-unit separately
from dotest (oops).
3. Fix formatting via git-clang-format.
Repository:
rL LLVM
https://reviews.llvm.org/D32757
Files:
include/lldb/Utility/TaskPool.h
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Utility/TaskPool.cpp
unittests/Utility/TaskPoolTest.cpp
Index: unittests/Utility/TaskPoolTest.cpp
===
--- unittests/Utility/TaskPoolTest.cpp
+++ unittests/Utility/TaskPoolTest.cpp
@@ -30,25 +30,14 @@
ASSERT_EQ(17, r[3]);
}
-TEST(TaskPoolTest, TaskRunner) {
- auto fn = [](int x) { return std::make_pair(x, x * x); };
-
- TaskRunner> tr;
- tr.AddTask(fn, 1);
- tr.AddTask(fn, 2);
- tr.AddTask(fn, 3);
- tr.AddTask(fn, 4);
-
- int count = 0;
- while (true) {
-auto f = tr.WaitForNextCompletedTask();
-if (!f.valid())
- break;
-
-++count;
-std::pair v = f.get();
-ASSERT_EQ(v.first * v.first, v.second);
- }
-
- ASSERT_EQ(4, count);
+TEST(TaskPoolTest, TaskMap) {
+ int data[4];
+ auto fn = [&data](int x) { data[x] = x * x; };
+
+ TaskMapOverInt(0, 4, fn);
+
+ ASSERT_EQ(data[0], 0);
+ ASSERT_EQ(data[1], 1);
+ ASSERT_EQ(data[2], 4);
+ ASSERT_EQ(data[3], 9);
}
Index: source/Utility/TaskPool.cpp
===
--- source/Utility/TaskPool.cpp
+++ source/Utility/TaskPool.cpp
@@ -73,3 +73,26 @@
f();
}
}
+
+void TaskMapOverInt(size_t begin, size_t end,
+std::function const &func) {
+ std::atomic idx{begin};
+ size_t num_workers =
+ std::min(end, std::thread::hardware_concurrency());
+
+ auto wrapper = [&idx, end, &func]() {
+while (true) {
+ size_t i = idx.fetch_add(1);
+ if (i >= end)
+break;
+ func(i);
+}
+ };
+
+ std::vector> futures;
+ futures.reserve(num_workers);
+ for (size_t i = 0; i < num_workers; i++)
+futures.push_back(TaskPool::AddTask(wrapper));
+ for (size_t i = 0; i < num_workers; i++)
+futures[i].wait();
+}
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1946,7 +1946,9 @@
std::vector type_index(num_compile_units);
std::vector namespace_index(num_compile_units);
-std::vector clear_cu_dies(num_compile_units, false);
+// std::vector might be implemented using bit test-and-set, so use
+// uint8_t instead.
+std::vector clear_cu_dies(num_compile_units, false);
auto parser_fn = [debug_info, &function_basename_index,
&function_fullname_index, &function_method_index,
&function_selector_index, &objc_class_selectors_index,
@@ -1963,22 +1965,18 @@
return cu_idx;
};
-auto extract_fn = [debug_info](uint32_t cu_idx) {
+auto extract_fn = [debug_info, &clear_cu_dies](uint32_t cu_idx) {
DWARFCompileUnit *dwarf_cu = debug_info->GetCompileUnitAtIndex(cu_idx);
if (dwarf_cu) {
// dwarf_cu->ExtractDIEsIfNeeded(false) will return zero if the
// DIEs for a compile unit have already been parsed.
-return std::make_pair(cu_idx, dwarf_cu->ExtractDIEsIfNeeded(false) > 1);
+if (dwarf_cu->ExtractDIEsIfNeeded(false) > 1)
+ clear_cu_dies[cu_idx] = true;
}
- return std::make_pair(cu_idx, false);
};
// Create a task runner that extracts dies for each DWARF compile unit in a
// separate thread
-TaskRunner> task_runner_extract;
-for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
- task_runner_extract.AddTask(extract_fn, cu_idx);
-
//--
// First figure out which compile units didn't have their DIEs already
// parsed and remember this. If no DIEs were parsed prior to this index
@@ -1988,48 +1986,37 @@
// a DIE in one compile unit refers to another and the indexes accesses
// those DIEs.
//--
-while (true) {
- auto f = task_runner_extract.WaitForNextCompletedTask();
- if (!f.valid())
-break;
- unsigned cu_idx;
- bool clear;
- std::tie(cu_idx, clear) = f.get();
- clear_cu_dies[cu_idx] = clear;
-}
+TaskMapOverInt(0, num_compile_units, extract_fn);
// Now create a task runner that can index each DWARF compile unit in a
// separate
// thread so we can index quickly.
-TaskRunner task_runner;
-for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
- task_runner.AddTask(parser_fn, cu_idx);
[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers
scott.smith added a comment. Last changes are cosmetic (though look big because I captured a different amount of context) so hopefully doesn't need a re-review. Can someone push them for me? Thank you! Repository: rL LLVM https://reviews.llvm.org/D32757 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32597: Initiate loading of shared libraries in parallel
scott.smith added a subscriber: ruiu.
scott.smith added inline comments.
Comment at:
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:530-560
+ struct loader {
+DYLDRendezvous::iterator I;
+ModuleSP m;
+std::shared_future f;
+ };
+ std::vector loaders(num_to_load);
+ llvm::ThreadPool load_pool(
zturner wrote:
> scott.smith wrote:
> > zturner wrote:
> > > zturner wrote:
> > > > I'm still unclear why we're still going down this route of adding a
> > > > very specialized instance of parallelism to this one place, that is
> > > > probably going to be less efficient than having a global thread pool
> > > > (as now you will have to spin up and destroy the entire thread pool
> > > > every time you go to load shared libraries).
> > > >
> > > > So, rather than keep repeating the same thing over and over, I went and
> > > > looked into this a little bit. It turns out LLD has a large library of
> > > > parallel algorithms in `lld/Include/lld/Parallel.h`. There is almost
> > > > nothing specific to LLD in any of these algorithms however, and they
> > > > should probably be moved to llvm support. Upon doing so, you will be
> > > > able to write this code as:
> > > >
> > > > ```
> > > > std::vector Modules(m_rendevous.size());
> > > > llvm::parallel_for_each(make_integer_range(0, m_rendevous.size()),
> > > > [this, &Modules](int I)
> > > > {
> > > > auto &R = m_rendevous[I];
> > > > Modules[I] = LoadModuleAtAddress(R.file_spec, R.link_addr,
> > > > R.base_addr, true);
> > > > });
> > > > for (const auto &M : Modules)
> > > >module_list.Append(M);
> > > > ```
> > > >
> > > > Please look into making this change.
> > > Note that this is, of course, exactly why I've been saying all along that
> > > we should not even be discussing this here. Had the discussion happened
> > > over on the LLVM side the first time I asked about this, someone could
> > > have mentioned this without me having to spend time looking into it.
> > As long as those routines allow you to recursively call those routines,
> > then great, go for it.
> >
> > The spinning up/tearing down of threads is not expensive, at least in my
> > measured use case.
> > As long as those routines allow you to recursively call those routines,
> > then great, go for it.
>
> Right, which is why I've asked at least 4 times for it to be looked into.
> That doesn't mean mean use the first thing you see and check it in right
> away, it means please see if there is some way to use existing code to solve
> the problem. If it turns out this doesn't allow recursive use, then a
> followup would be to understand why not, and if it is hard to change.
>
> So again, please look into this and see if it will work for your needs.
It seems there are two possible long term fixes:
1. Separate thread pool
2. Make parallel_for support recursive use.
Personally I'm in favor of 2. @ruiu brought it up as a preferred goal too.
The q is then how to proceed in the short term:
1. Wait for parallel_for to land in llvm, make it support recursion (hopefully
Windows ConcRT supports it too?), then convert this change and commit.
2. Change TaskMapOverInt to take a ThreadPool, then have it use a private
ThreadPool for now, then easily convert to the #1 above later.
3. Change TaskMapOverInt to support recursion now (which will also require
changes to lldb::TaskPool, IMO), then do #1 above once it lands. Advantage is
we prove it works, which hopefully makes moving it upstream easier.
Any preferences on how to proceed?
Repository:
rL LLVM
https://reviews.llvm.org/D32597
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32820: Parallelize demangling
scott.smith added inline comments.
Comment at: source/Symbol/Symtab.cpp:257
-// The "const char *" in "class_contexts" must come from a
-// ConstString::GetCString()
-std::set class_contexts;
-UniqueCStringMap mangled_name_to_index;
-std::vector symbol_contexts(num_symbols, nullptr);
-
-for (entry.value = 0; entry.value < num_symbols; ++entry.value) {
- const Symbol *symbol = &m_symbols[entry.value];
+auto symbol_fn = [&states, this](size_t value) {
+ const Symbol *symbol = &m_symbols[value];
labath wrote:
> The function looks big enough to deserve a name. (== Please move the lambda
> out of line)
ok but now's your chance to review it as a diff rather than a sea of red and
green
Repository:
rL LLVM
https://reviews.llvm.org/D32820
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32823: Remove an expensive lock from Timer
scott.smith updated this revision to Diff 97973.
scott.smith marked 2 inline comments as done.
scott.smith added a comment.
remove same-name-different-site support from Timer::Category
Repository:
rL LLVM
https://reviews.llvm.org/D32823
Files:
include/lldb/Core/Timer.h
source/API/SystemInitializerFull.cpp
source/Commands/CommandObjectTarget.cpp
source/Core/Disassembler.cpp
source/Core/Mangled.cpp
source/Core/Module.cpp
source/Core/Timer.cpp
source/Host/common/Symbols.cpp
source/Initialization/SystemInitializerCommon.cpp
source/Interpreter/CommandInterpreter.cpp
source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugPubnames.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
source/Symbol/DWARFCallFrameInfo.cpp
source/Symbol/ObjectFile.cpp
source/Symbol/Symtab.cpp
source/Target/Target.cpp
source/Target/TargetList.cpp
unittests/Core/TimerTest.cpp
Index: unittests/Core/TimerTest.cpp
===
--- unittests/Core/TimerTest.cpp
+++ unittests/Core/TimerTest.cpp
@@ -18,7 +18,8 @@
TEST(TimerTest, CategoryTimes) {
Timer::ResetCategoryTimes();
{
-Timer t("CAT1", "");
+static Timer::Category tcat("CAT1");
+Timer t(tcat, "");
std::this_thread::sleep_for(std::chrono::milliseconds(10));
}
StreamString ss;
@@ -32,14 +33,18 @@
TEST(TimerTest, CategoryTimesNested) {
Timer::ResetCategoryTimes();
{
-Timer t1("CAT1", "");
+static Timer::Category tcat1("CAT1");
+Timer t1(tcat1, "");
std::this_thread::sleep_for(std::chrono::milliseconds(10));
-Timer t2("CAT1", "");
+// Explicitly testing the same category as above.
+Timer t2(tcat1, "");
std::this_thread::sleep_for(std::chrono::milliseconds(10));
}
StreamString ss;
Timer::DumpCategoryTimes(&ss);
double seconds;
+ // It should only appear once.
+ ASSERT_EQ(ss.GetString().count("CAT1"), 1U);
ASSERT_EQ(1, sscanf(ss.GetData(), "%lf sec for CAT1", &seconds));
EXPECT_LT(0.002, seconds);
EXPECT_GT(0.2, seconds);
@@ -48,9 +53,11 @@
TEST(TimerTest, CategoryTimes2) {
Timer::ResetCategoryTimes();
{
-Timer t1("CAT1", "");
+static Timer::Category tcat1("CAT1");
+Timer t1(tcat1, "");
std::this_thread::sleep_for(std::chrono::milliseconds(100));
-Timer t2("CAT2", "");
+static Timer::Category tcat2("CAT2");
+Timer t2(tcat2, "");
std::this_thread::sleep_for(std::chrono::milliseconds(10));
}
StreamString ss;
Index: source/Target/TargetList.cpp
===
--- source/Target/TargetList.cpp
+++ source/Target/TargetList.cpp
@@ -325,10 +325,10 @@
lldb::PlatformSP &platform_sp,
lldb::TargetSP &target_sp,
bool is_dummy_target) {
- Timer scoped_timer(LLVM_PRETTY_FUNCTION,
- "TargetList::CreateTarget (file = '%s', arch = '%s')",
- user_exe_path.str().c_str(),
- specified_arch.GetArchitectureName());
+ static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
+ Timer scoped_timer(
+ func_cat, "TargetList::CreateTarget (file = '%s', arch = '%s')",
+ user_exe_path.str().c_str(), specified_arch.GetArchitectureName());
Error error;
ArchSpec arch(specified_arch);
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1235,7 +1235,8 @@
ClearModules(false);
if (executable_sp) {
-Timer scoped_timer(LLVM_PRETTY_FUNCTION,
+static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
+Timer scoped_timer(func_cat,
"Target::SetExecutableModule (executable = '%s')",
executable_sp->GetFileSpec().GetPath().c_str());
Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -220,7 +220,8 @@
// Protected function, no need to lock mutex...
if (!m_name_indexes_computed) {
m_name_indexes_computed = true;
-Timer scoped_timer(LLVM_PRETTY_FUNCTION, "%s", LLVM_PRETTY_FUNCTION);
+static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
+Timer scoped_timer(func_cat, "%s
[Lldb-commits] [PATCH] D32823: Remove an expensive lock from Timer
scott.smith added inline comments.
Comment at: unittests/Core/TimerTest.cpp:39
std::this_thread::sleep_for(std::chrono::milliseconds(10));
-Timer t2("CAT1", "");
+// Explicitly testing the same category as above.
+static Timer::Category tcat1b("CAT1");
labath wrote:
> Do we actually need to support that? Since we already have a nice Category
> object then I think we should use that as a unique key for everything
> (including printing). There no reason why the category needs to have local
> scope. If it really needs to be used from multiple places, we can expose the
> object at a higher scope. For the tests, this could be done by having a
> fixture which creates the categories in the SetUpTestCase static function.
> Alternatively, we could have Category object be based on llvm::ManagedStatic,
> so it would go away when you call llvm_shutdown, which would enable us to
> have truly isolated and leak-free tests. (Right now we don't call
> llvm_shutdown in the main app as we cannot shutdown cleanly, but it would be
> great if one day we could.)
I don't think so.
git grep Timer::Category | grep -v LLVM_PRETTY_FUNCTION
shows one place (other than the unit test) where the name is explicitly set,
and that's because that function already has its own scoped timer. All the
other places are unique (one per function).
The test might be testing for recursion-safe timers, in which case I can just
change the declaration of t2:
Timer t2(tcat1a, "")
so it uses the same category object.
Repository:
rL LLVM
https://reviews.llvm.org/D32823
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32820: Parallelize demangling
scott.smith marked an inline comment as done. scott.smith added a comment. In https://reviews.llvm.org/D32820#747309, @clayborg wrote: > What are the measured improvements here? We can't slow things down on any > platforms. I know MacOS didn't respond well to making demangling run in > parallel. I want to see some numbers here. And please don't quote numbers > with tcmalloc or any special allocator unless you have a patch in LLDB > already to make this a build option. Without tcmalloc, on Ubuntu 14.04, 40 core VM: 13% With tcmalloc, on Ubuntu 14.04, 40 core VM: 24% (built using cmake ... -DCMAKE_EXE_LINKER_FLAGS=-ltcmalloc_minimal, which amazingly only works when building with clang, not gcc...) I don't have access to a Mac, and of course YMMV depending on the application. so yeah, it's a bigger improvement with tcmalloc. Interestingly, I tried adding back the demangler improvements I had queued up (which reduced memory allocations), and they didn't matter much, which makes me think this is contention allocating const strings. I could be wrong though. By far the biggest performance improvement I have queued up is loading the shared libraries in parallel (https://reviews.llvm.org/D32597), but that's waiting on pulling parallel_for_each from LLD into LLVM (I think). If you're really leery of this change, I could just make the structural changes to allow parallelization, and then keep a small patch internally to enable it. Or enabling it could be platform dependent. Or ... ? Comment at: source/Symbol/Symtab.cpp:233-239 + // Don't let trampolines get into the lookup by name map + // If we ever need the trampoline symbols to be searchable by name + // we can remove this and then possibly add a new bool to any of the + // Symtab functions that lookup symbols by name to indicate if they + // want trampolines. + if (symbol.IsTrampoline()) +return; clayborg wrote: > Not being able to search for symbols by name when they are trampolines? If > you lookup symbols by name I would expect things not to fail and I would > expect that I would get all the answers, not just ones that are omitted for > performance reasons. I would also not expect to have to specify extra flags. > Please remove This is just moved code, not new code. You can use the phabricator history tool above to diff baseline against #97908, and see that the only change I made was continue->return, due to changing it from a loop to a lambda (now a separate function). This is why I pubished D32708 separately - I wanted to separate the functional change from the structural change. Repository: rL LLVM https://reviews.llvm.org/D32820 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32823: Remove an expensive lock from Timer
scott.smith added a comment. Can someone please commit this? Thank you! Repository: rL LLVM https://reviews.llvm.org/D32823 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions
scott.smith created this revision.
scott.smith added a project: LLDB.
Herald added a subscriber: mgorny.
Remove the thread pool and for_each-like iteration functions.
Keep RunTasks, which has no analog in llvm::parallel, but implement it using
llvm::parallel.
Repository:
rL LLVM
https://reviews.llvm.org/D33246
Files:
include/lldb/Utility/TaskPool.h
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Utility/CMakeLists.txt
source/Utility/TaskPool.cpp
unittests/Utility/TaskPoolTest.cpp
Index: unittests/Utility/TaskPoolTest.cpp
===
--- unittests/Utility/TaskPoolTest.cpp
+++ unittests/Utility/TaskPoolTest.cpp
@@ -2,20 +2,6 @@
#include "lldb/Utility/TaskPool.h"
-TEST(TaskPoolTest, AddTask) {
- auto fn = [](int x) { return x * x + 1; };
-
- auto f1 = TaskPool::AddTask(fn, 1);
- auto f2 = TaskPool::AddTask(fn, 2);
- auto f3 = TaskPool::AddTask(fn, 3);
- auto f4 = TaskPool::AddTask(fn, 4);
-
- ASSERT_EQ(10, f3.get());
- ASSERT_EQ(2, f1.get());
- ASSERT_EQ(17, f4.get());
- ASSERT_EQ(5, f2.get());
-}
-
TEST(TaskPoolTest, RunTasks) {
std::vector r(4);
@@ -29,15 +15,3 @@
ASSERT_EQ(10, r[2]);
ASSERT_EQ(17, r[3]);
}
-
-TEST(TaskPoolTest, TaskMap) {
- int data[4];
- auto fn = [&data](int x) { data[x] = x * x; };
-
- TaskMapOverInt(0, 4, fn);
-
- ASSERT_EQ(data[0], 0);
- ASSERT_EQ(data[1], 1);
- ASSERT_EQ(data[2], 4);
- ASSERT_EQ(data[3], 9);
-}
Index: source/Utility/TaskPool.cpp
===
--- source/Utility/TaskPool.cpp
+++ /dev/null
@@ -1,98 +0,0 @@
-//===- TaskPool.cpp -*- C++ -*-===//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===--===//
-
-#include "lldb/Utility/TaskPool.h"
-
-#include // for uint32_t
-#include// for queue
-#include // for thread
-
-namespace {
-class TaskPoolImpl {
-public:
- static TaskPoolImpl &GetInstance();
-
- void AddTask(std::function &&task_fn);
-
-private:
- TaskPoolImpl();
-
- static void Worker(TaskPoolImpl *pool);
-
- std::queue> m_tasks;
- std::mutex m_tasks_mutex;
- uint32_t m_thread_count;
-};
-
-} // end of anonymous namespace
-
-TaskPoolImpl &TaskPoolImpl::GetInstance() {
- static TaskPoolImpl g_task_pool_impl;
- return g_task_pool_impl;
-}
-
-void TaskPool::AddTaskImpl(std::function &&task_fn) {
- TaskPoolImpl::GetInstance().AddTask(std::move(task_fn));
-}
-
-TaskPoolImpl::TaskPoolImpl() : m_thread_count(0) {}
-
-void TaskPoolImpl::AddTask(std::function &&task_fn) {
- static const uint32_t max_threads = std::thread::hardware_concurrency();
-
- std::unique_lock lock(m_tasks_mutex);
- m_tasks.emplace(std::move(task_fn));
- if (m_thread_count < max_threads) {
-m_thread_count++;
-// Note that this detach call needs to happen with the m_tasks_mutex held.
-// This prevents the thread
-// from exiting prematurely and triggering a linux libc bug
-// (https://sourceware.org/bugzilla/show_bug.cgi?id=19951).
-std::thread(Worker, this).detach();
- }
-}
-
-void TaskPoolImpl::Worker(TaskPoolImpl *pool) {
- while (true) {
-std::unique_lock lock(pool->m_tasks_mutex);
-if (pool->m_tasks.empty()) {
- pool->m_thread_count--;
- break;
-}
-
-std::function f = pool->m_tasks.front();
-pool->m_tasks.pop();
-lock.unlock();
-
-f();
- }
-}
-
-void TaskMapOverInt(size_t begin, size_t end,
-std::function const &func) {
- std::atomic idx{begin};
- size_t num_workers =
- std::min(end, std::thread::hardware_concurrency());
-
- auto wrapper = [&idx, end, &func]() {
-while (true) {
- size_t i = idx.fetch_add(1);
- if (i >= end)
-break;
- func(i);
-}
- };
-
- std::vector> futures;
- futures.reserve(num_workers);
- for (size_t i = 0; i < num_workers; i++)
-futures.push_back(TaskPool::AddTask(wrapper));
- for (size_t i = 0; i < num_workers; i++)
-futures[i].wait();
-}
Index: source/Utility/CMakeLists.txt
===
--- source/Utility/CMakeLists.txt
+++ source/Utility/CMakeLists.txt
@@ -26,7 +26,6 @@
StringExtractorGDBRemote.cpp
StringLexer.cpp
StringList.cpp
- TaskPool.cpp
TildeExpressionResolver.cpp
UserID.cpp
UriParser.cpp
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -11,6 +11,7 @@
// Other libraries and framework includes
#include "llvm/Support/Casting.h"
+#include "llvm/Support/Parallel.h"
#include "llvm/Support/Threading.h"
#include "lldb/Core/ArchSpec
[Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions
scott.smith added inline comments.
Comment at: include/lldb/Utility/TaskPool.h:18-20
+ std::function cbs[sizeof...(T)]{tasks...};
+ llvm::parallel::for_each_n(llvm::parallel::par, static_cast(0),
+ sizeof...(T), [&cbs](size_t idx) { cbs[idx](); });
zturner wrote:
> I'm not sure this is the most efficient implementation. `std::function` has
> pretty poor performance, and there might be no need to even convert
> everything to `std::function` to begin with. You could make this a bit
> better by using `llvm::function_ref` instead.
>
> That said, I wonder if it's worth adding a function like this to
> `llvm::TaskGroup`? And you could just enqueue all the tasks, rather than
> `for_each_n`. Not sure if there would be a different in practice, what do
> you think?
I'm not too worried about std::function vs llvm::function_ref; it isn't called
often, and we still need allocations for the tasks that get enqueued. That
said, there's no reason *to* use std::function, so I'll cahnge it.
I like using for_each_n mostly to regularize the interface. For example,
for_each_n/for_each can then optimize the type of TaskGroup it creates to
ensure that it gets the right # of threads right away, rather than spawning up
enough for full hardware concurrency. Or, if there are a lot of tasks
(unlikely, but possible), then for_each can change to a model of enqueueing one
task per thread, and having that thread loop using std::atomic to increment the
iterator, which reduces allocations in TaskGroup and reduces lock contention
(assuming TaskGroup doesn't use a lock free queue).
i.e. the more things funnel through a single interface, the more we benefit
from optimizing that one implementation.
Also it means we can have for_each_n manage TaskGroups itself (maybe keeping
one around for repeated use, then creating more as needed to support recursion,
etc (more on that later)).
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1995-1996
//--
-TaskMapOverInt(0, num_compile_units, extract_fn);
+llvm::parallel::for_each_n(llvm::parallel::par, 0U, num_compile_units,
+ extract_fn);
zturner wrote:
> What did you decide about the recursive parallelism? I don't know if that
> works yet using LLVM's default executor.
1. This code doesn't care.
2. It looks like it works, since (I think) for_each creates a separate
TaskGroup for each call.
3. However I got a deadlock when using this for parallelizing the dynamic
library loading itself, which used to work. That could either be due to other
code changes, some oversight on my part, or it could be that for_each_n doesn't
actually support recursion - which means that I misunderstood for_each_n. So I
have more work to do...
Repository:
rL LLVM
https://reviews.llvm.org/D33246
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions
scott.smith added inline comments.
Comment at: include/lldb/Utility/TaskPool.h:18-20
+ std::function cbs[sizeof...(T)]{tasks...};
+ llvm::parallel::for_each_n(llvm::parallel::par, static_cast(0),
+ sizeof...(T), [&cbs](size_t idx) { cbs[idx](); });
scott.smith wrote:
> zturner wrote:
> > I'm not sure this is the most efficient implementation. `std::function`
> > has pretty poor performance, and there might be no need to even convert
> > everything to `std::function` to begin with. You could make this a bit
> > better by using `llvm::function_ref` instead.
> >
> > That said, I wonder if it's worth adding a function like this to
> > `llvm::TaskGroup`? And you could just enqueue all the tasks, rather than
> > `for_each_n`. Not sure if there would be a different in practice, what do
> > you think?
> I'm not too worried about std::function vs llvm::function_ref; it isn't
> called often, and we still need allocations for the tasks that get enqueued.
> That said, there's no reason *to* use std::function, so I'll cahnge it.
>
> I like using for_each_n mostly to regularize the interface. For example,
> for_each_n/for_each can then optimize the type of TaskGroup it creates to
> ensure that it gets the right # of threads right away, rather than spawning
> up enough for full hardware concurrency. Or, if there are a lot of tasks
> (unlikely, but possible), then for_each can change to a model of enqueueing
> one task per thread, and having that thread loop using std::atomic to
> increment the iterator, which reduces allocations in TaskGroup and reduces
> lock contention (assuming TaskGroup doesn't use a lock free queue).
>
> i.e. the more things funnel through a single interface, the more we benefit
> from optimizing that one implementation.
>
> Also it means we can have for_each_n manage TaskGroups itself (maybe keeping
> one around for repeated use, then creating more as needed to support
> recursion, etc (more on that later)).
>
oh, and I wouldn't be surprised if there's a better home in llvm. I'm fine
with moving it. I doubt it should go in llvm::parallel, since that seems like
it's trying to be similar to std::parallel (though note for_each_n is
incompatible, since it should take Iter,Size instead of Type,Type, and it tries
to dereference Iter, which means you can't pass in a number like all the
callsites do. I tried fixing that but failed due to the deref assumption, and
the LLD dependency).
It's small enough that it does seem like it should fit under something else
rather than be standalone; I'm open to suggestions.
Repository:
rL LLVM
https://reviews.llvm.org/D33246
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions
scott.smith added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1995-1996 //-- -TaskMapOverInt(0, num_compile_units, extract_fn); +llvm::parallel::for_each_n(llvm::parallel::par, 0U, num_compile_units, + extract_fn); scott.smith wrote: > zturner wrote: > > What did you decide about the recursive parallelism? I don't know if that > > works yet using LLVM's default executor. > 1. This code doesn't care. > 2. It looks like it works, since (I think) for_each creates a separate > TaskGroup for each call. > 3. However I got a deadlock when using this for parallelizing the dynamic > library loading itself, which used to work. That could either be due to > other code changes, some oversight on my part, or it could be that for_each_n > doesn't actually support recursion - which means that I misunderstood > for_each_n. So I have more work to do... On further inspection, llvm::parallel does not support recursion, since TaskGroup uses a single static Executor, and provides no way to override that (and besides, there's no way to pass parameters from for_each_n to the TaskGroup). That's fixable though, by making the default executor a thread local variable, so that worker threads can enqueue work to a different executor. Repository: rL LLVM https://reviews.llvm.org/D33246 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32820: Parallelize demangling
scott.smith marked an inline comment as done. scott.smith added a comment. In https://reviews.llvm.org/D32820#759141, @emaste wrote: > > Without tcmalloc, on Ubuntu 14.04, 40 core VM: 13% > > With tcmalloc, on Ubuntu 14.04, 40 core VM: 24% (built using cmake ... > > -DCMAKE_EXE_LINKER_FLAGS=-ltcmalloc_minimal, which amazingly only works > > when building with clang, not gcc...) > > Do you have a brief set of steps you use for benchmarking? I'd like to > compare on FreeBSD using a similar test. time lldb -b -o 'b main' -o 'run' /my/program (sometimes I use 'perf stat' instead of time; I don't know if FreeBSD has something similar to Linux's perf - basically instruction count, cycle count, branch counts and mispredict rate, etc.) my program happens to have a lot of symbols and a lot of libraries. I tried this benchmark with lldb itself, and all but one of my changes have no effect because lldb only links in one large library, so YMMV depending on the application. Repository: rL LLVM https://reviews.llvm.org/D32820 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
