<div dir="ltr">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).<div><br></div><div>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).</div><div><br></div><div>If you really want to use some library for it then we should look into <span style="font-size:13px;line-height:19.5px">libdispatch or find something similar, but I don't think we want to add a new dependency to LLDB.</span></div><div><span style="font-size:13px;line-height:19.5px"><br></span></div><div><span style="font-size:13px;line-height:19.5px">Tamas</span></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 20, 2015 at 5:00 PM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">This is kind of why I pushed back against this in the first place :-/ It's hard to get right. <div><br></div><div>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).</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 20, 2015 at 8:45 AM Tamas Berghammer via lldb-commits <<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: tberghammer<br>
Date: Tue Oct 20 10:43:40 2015<br>
New Revision: 250832<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=250832&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=250832&view=rev</a><br>
Log:<br>
Revert "Make dwarf parsing multi-threaded"<br>
<br>
Revert it bacuse it introduces several race condition detected by<br>
the build bots.<br>
<br>
This reverts commit 5107a5ebdb7c4571a30a7098b40bf8098b678447.<br>
<br>
Modified:<br>
lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp<br>
lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h<br>
lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp<br>
<br>
Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp?rev=250832&r1=250831&r2=250832&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp?rev=250832&r1=250831&r2=250832&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp (original)<br>
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp Tue Oct 20 10:43:40 2015<br>
@@ -83,14 +83,3 @@ NameToDIE::ForEach (std::function <bool(<br>
break;<br>
}<br>
}<br>
-<br>
-void<br>
-NameToDIE::Append (const NameToDIE& other)<br>
-{<br>
- const uint32_t size = other.m_map.GetSize();<br>
- for (uint32_t i = 0; i < size; ++i)<br>
- {<br>
- m_map.Append(other.m_map.GetCStringAtIndexUnchecked (i),<br>
- other.m_map.GetValueAtIndexUnchecked (i));<br>
- }<br>
-}<br>
<br>
Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h?rev=250832&r1=250831&r2=250832&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h?rev=250832&r1=250831&r2=250832&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h (original)<br>
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/NameToDIE.h Tue Oct 20 10:43:40 2015<br>
@@ -38,9 +38,6 @@ public:<br>
Insert (const lldb_private::ConstString& name, const DIERef& die_ref);<br>
<br>
void<br>
- Append (const NameToDIE& other);<br>
-<br>
- void<br>
Finalize();<br>
<br>
size_t<br>
<br>
Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp?rev=250832&r1=250831&r2=250832&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp?rev=250832&r1=250831&r2=250832&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (original)<br>
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp Tue Oct 20 10:43:40 2015<br>
@@ -50,8 +50,6 @@<br>
<br>
#include "lldb/Target/Language.h"<br>
<br>
-#include "lldb/Utility/TaskPool.h"<br>
-<br>
#include "DWARFASTParser.h"<br>
#include "DWARFCompileUnit.h"<br>
#include "DWARFDebugAbbrev.h"<br>
@@ -2037,72 +2035,40 @@ SymbolFileDWARF::Index ()<br>
"SymbolFileDWARF::Index (%s)",<br>
GetObjectFile()->GetFileSpec().GetFilename().AsCString("<Unknown>"));<br>
<br>
-<br>
-<br>
DWARFDebugInfo* debug_info = DebugInfo();<br>
if (debug_info)<br>
{<br>
+ uint32_t cu_idx = 0;<br>
const uint32_t num_compile_units = GetNumCompileUnits();<br>
- std::vector<NameToDIE> function_basename_index(num_compile_units);<br>
- std::vector<NameToDIE> function_fullname_index(num_compile_units);<br>
- std::vector<NameToDIE> function_method_index(num_compile_units);<br>
- std::vector<NameToDIE> function_selector_index(num_compile_units);<br>
- std::vector<NameToDIE> objc_class_selectors_index(num_compile_units);<br>
- std::vector<NameToDIE> global_index(num_compile_units);<br>
- std::vector<NameToDIE> type_index(num_compile_units);<br>
- std::vector<NameToDIE> namespace_index(num_compile_units);<br>
-<br>
- auto parser_fn = [this,<br>
- debug_info,<br>
- &function_basename_index,<br>
- &function_fullname_index,<br>
- &function_method_index,<br>
- &function_selector_index,<br>
- &objc_class_selectors_index,<br>
- &global_index,<br>
- &type_index,<br>
- &namespace_index](uint32_t cu_idx)<br>
+ for (cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)<br>
{<br>
DWARFCompileUnit* dwarf_cu = debug_info->GetCompileUnitAtIndex(cu_idx);<br>
- bool clear_dies = dwarf_cu->ExtractDIEsIfNeeded(false) > 1;<br>
<br>
- dwarf_cu->Index(function_basename_index[cu_idx],<br>
- function_fullname_index[cu_idx],<br>
- function_method_index[cu_idx],<br>
- function_selector_index[cu_idx],<br>
- objc_class_selectors_index[cu_idx],<br>
- global_index[cu_idx],<br>
- type_index[cu_idx],<br>
- namespace_index[cu_idx]);<br>
+ bool clear_dies = dwarf_cu->ExtractDIEsIfNeeded (false) > 1;<br>
<br>
+ dwarf_cu->Index (m_function_basename_index,<br>
+ m_function_fullname_index,<br>
+ m_function_method_index,<br>
+ m_function_selector_index,<br>
+ m_objc_class_selectors_index,<br>
+ m_global_index,<br>
+ m_type_index,<br>
+ m_namespace_index);<br>
+<br>
// Keep memory down by clearing DIEs if this generate function<br>
// caused them to be parsed<br>
if (clear_dies)<br>
- dwarf_cu->ClearDIEs(true);<br>
- };<br>
-<br>
- std::vector<std::future<void>> results;<br>
- for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)<br>
- results.emplace_back(TaskPool::AddTask(parser_fn, cu_idx));<br>
- for (auto& f : results)<br>
- f.wait();<br>
-<br>
- auto merge_fn = [](NameToDIE& target, const std::vector<NameToDIE>& sources)<br>
- {<br>
- for (const auto& src : sources)<br>
- target.Append(src);<br>
- target.Finalize();<br>
- };<br>
-<br>
- TaskPool::RunTasks(<br>
- [&]() { merge_fn(m_function_basename_index, function_basename_index); },<br>
- [&]() { merge_fn(m_function_fullname_index, function_fullname_index); },<br>
- [&]() { merge_fn(m_function_method_index, function_method_index); },<br>
- [&]() { merge_fn(m_function_selector_index, function_selector_index); },<br>
- [&]() { merge_fn(m_objc_class_selectors_index, objc_class_selectors_index); },<br>
- [&]() { merge_fn(m_global_index, global_index); },<br>
- [&]() { merge_fn(m_type_index, type_index); },<br>
- [&]() { merge_fn(m_namespace_index, namespace_index); });<br>
+ dwarf_cu->ClearDIEs (true);<br>
+ }<br>
+<br>
+ m_function_basename_index.Finalize();<br>
+ m_function_fullname_index.Finalize();<br>
+ m_function_method_index.Finalize();<br>
+ m_function_selector_index.Finalize();<br>
+ m_objc_class_selectors_index.Finalize();<br>
+ m_global_index.Finalize();<br>
+ m_type_index.Finalize();<br>
+ m_namespace_index.Finalize();<br>
<br>
#if defined (ENABLE_DEBUG_PRINTF)<br>
StreamFile s(stdout, false);<br>
<br>
<br>
_______________________________________________<br>
lldb-commits mailing list<br>
<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits</a><br>
</blockquote></div>
</blockquote></div></div>