[Lldb-commits] [lldb] r250832 - Revert "Make dwarf parsing multi-threaded"
Tamas Berghammer via lldb-commits
lldb-commits at lists.llvm.org
Tue Oct 20 09:15:01 PDT 2015
The race condition made me to revert this change is inside the dwarf
parsing code (and in some other part of LLDB), not in the thread pool
implementation. I run the unit-tests I wrote under TSAN and they all run
cleanly so I don't really expect any race condition coming from that part
of the code. If you check the implementation it is very simple so I think
it is fairly easy to reason about it (1 mutex guarding all member variable
of ThreadPoolImpl).
Writing code to manage the number of threads we are using for parsing the
compile units is possible in this concrete scenario but I don't think it is
a good idea if we consider that we want to make LLDB multi threaded in
several other places (Greg mentioned lookup inside ModuleList, I want to
make DebugAbbrev and Symtab parsing also multi threaded, etc.). Managing
the number of threads we want to use at each place is more error prone in
my opinion (we have to get it right all the time, not only in 1 utility
class).
If you really want to use some library for it then we should look into
libdispatch
or find something similar, but I don't think we want to add a new
dependency to LLDB.
Tamas
On Tue, Oct 20, 2015 at 5:00 PM Zachary Turner <zturner at google.com> wrote:
> This is kind of why I pushed back against this in the first place :-/
> It's hard to get right.
>
> I still wish we could find a way to use standard library facilities for
> this, either by splitting the compile units into hardware_concurrency
> chunks, or kicking off no more than hardware_concurrency compile units at a
> time, and having them all share a mutex around a queue of compilation
> units, so that when each one's work is finished, it can queue the next item
> (if one still exists).
>
> On Tue, Oct 20, 2015 at 8:45 AM Tamas Berghammer via lldb-commits <
> lldb-commits at lists.llvm.org> wrote:
>
>> Author: tberghammer
>> Date: Tue Oct 20 10:43:40 2015
>> New Revision: 250832
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=250832&view=rev
>> Log:
>> Revert "Make dwarf parsing multi-threaded"
>>
>> Revert it bacuse it introduces several race condition detected by
>> the build bots.
>>
>> This reverts commit 5107a5ebdb7c4571a30a7098b40bf8098b678447.
>>
>> Modified:
>> lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
>> lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h
>> lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
>>
>> Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp?rev=250832&r1=250831&r2=250832&view=diff
>>
>> ==============================================================================
>> --- lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp (original)
>> +++ lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp Tue Oct 20
>> 10:43:40 2015
>> @@ -83,14 +83,3 @@ NameToDIE::ForEach (std::function <bool(
>> break;
>> }
>> }
>> -
>> -void
>> -NameToDIE::Append (const NameToDIE& other)
>> -{
>> - const uint32_t size = other.m_map.GetSize();
>> - for (uint32_t i = 0; i < size; ++i)
>> - {
>> - m_map.Append(other.m_map.GetCStringAtIndexUnchecked (i),
>> - other.m_map.GetValueAtIndexUnchecked (i));
>> - }
>> -}
>>
>> Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h?rev=250832&r1=250831&r2=250832&view=diff
>>
>> ==============================================================================
>> --- lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h (original)
>> +++ lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h Tue Oct 20
>> 10:43:40 2015
>> @@ -38,9 +38,6 @@ public:
>> Insert (const lldb_private::ConstString& name, const DIERef&
>> die_ref);
>>
>> void
>> - Append (const NameToDIE& other);
>> -
>> - void
>> Finalize();
>>
>> size_t
>>
>> Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp?rev=250832&r1=250831&r2=250832&view=diff
>>
>> ==============================================================================
>> --- lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
>> (original)
>> +++ lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp Tue
>> Oct 20 10:43:40 2015
>> @@ -50,8 +50,6 @@
>>
>> #include "lldb/Target/Language.h"
>>
>> -#include "lldb/Utility/TaskPool.h"
>> -
>> #include "DWARFASTParser.h"
>> #include "DWARFCompileUnit.h"
>> #include "DWARFDebugAbbrev.h"
>> @@ -2037,72 +2035,40 @@ SymbolFileDWARF::Index ()
>> "SymbolFileDWARF::Index (%s)",
>>
>> GetObjectFile()->GetFileSpec().GetFilename().AsCString("<Unknown>"));
>>
>> -
>> -
>> DWARFDebugInfo* debug_info = DebugInfo();
>> if (debug_info)
>> {
>> + uint32_t cu_idx = 0;
>> const uint32_t num_compile_units = GetNumCompileUnits();
>> - std::vector<NameToDIE>
>> function_basename_index(num_compile_units);
>> - std::vector<NameToDIE>
>> function_fullname_index(num_compile_units);
>> - std::vector<NameToDIE> function_method_index(num_compile_units);
>> - std::vector<NameToDIE>
>> function_selector_index(num_compile_units);
>> - std::vector<NameToDIE>
>> objc_class_selectors_index(num_compile_units);
>> - std::vector<NameToDIE> global_index(num_compile_units);
>> - std::vector<NameToDIE> type_index(num_compile_units);
>> - std::vector<NameToDIE> namespace_index(num_compile_units);
>> -
>> - auto parser_fn = [this,
>> - debug_info,
>> - &function_basename_index,
>> - &function_fullname_index,
>> - &function_method_index,
>> - &function_selector_index,
>> - &objc_class_selectors_index,
>> - &global_index,
>> - &type_index,
>> - &namespace_index](uint32_t cu_idx)
>> + for (cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
>> {
>> DWARFCompileUnit* dwarf_cu =
>> debug_info->GetCompileUnitAtIndex(cu_idx);
>> - bool clear_dies = dwarf_cu->ExtractDIEsIfNeeded(false) > 1;
>>
>> - dwarf_cu->Index(function_basename_index[cu_idx],
>> - function_fullname_index[cu_idx],
>> - function_method_index[cu_idx],
>> - function_selector_index[cu_idx],
>> - objc_class_selectors_index[cu_idx],
>> - global_index[cu_idx],
>> - type_index[cu_idx],
>> - namespace_index[cu_idx]);
>> + bool clear_dies = dwarf_cu->ExtractDIEsIfNeeded (false) > 1;
>>
>> + dwarf_cu->Index (m_function_basename_index,
>> + m_function_fullname_index,
>> + m_function_method_index,
>> + m_function_selector_index,
>> + m_objc_class_selectors_index,
>> + m_global_index,
>> + m_type_index,
>> + m_namespace_index);
>> +
>> // Keep memory down by clearing DIEs if this generate
>> function
>> // caused them to be parsed
>> if (clear_dies)
>> - dwarf_cu->ClearDIEs(true);
>> - };
>> -
>> - std::vector<std::future<void>> results;
>> - for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
>> - results.emplace_back(TaskPool::AddTask(parser_fn, cu_idx));
>> - for (auto& f : results)
>> - f.wait();
>> -
>> - auto merge_fn = [](NameToDIE& target, const
>> std::vector<NameToDIE>& sources)
>> - {
>> - for (const auto& src : sources)
>> - target.Append(src);
>> - target.Finalize();
>> - };
>> -
>> - TaskPool::RunTasks(
>> - [&]() { merge_fn(m_function_basename_index,
>> function_basename_index); },
>> - [&]() { merge_fn(m_function_fullname_index,
>> function_fullname_index); },
>> - [&]() { merge_fn(m_function_method_index,
>> function_method_index); },
>> - [&]() { merge_fn(m_function_selector_index,
>> function_selector_index); },
>> - [&]() { merge_fn(m_objc_class_selectors_index,
>> objc_class_selectors_index); },
>> - [&]() { merge_fn(m_global_index, global_index); },
>> - [&]() { merge_fn(m_type_index, type_index); },
>> - [&]() { merge_fn(m_namespace_index, namespace_index); });
>> + dwarf_cu->ClearDIEs (true);
>> + }
>> +
>> + m_function_basename_index.Finalize();
>> + m_function_fullname_index.Finalize();
>> + m_function_method_index.Finalize();
>> + m_function_selector_index.Finalize();
>> + m_objc_class_selectors_index.Finalize();
>> + m_global_index.Finalize();
>> + m_type_index.Finalize();
>> + m_namespace_index.Finalize();
>>
>> #if defined (ENABLE_DEBUG_PRINTF)
>> StreamFile s(stdout, false);
>>
>>
>> _______________________________________________
>> lldb-commits mailing list
>> lldb-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20151020/91a31219/attachment.html>
More information about the lldb-commits
mailing list