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
==
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, an
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 performa
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
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/CPl
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/I
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 do
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
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!
Repo
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
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/
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.cp
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().si
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 a
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/D
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
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
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(){ret
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
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 wit
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
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 th
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
i
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
includ
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/Symbo
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/Co
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
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 oc
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.cs
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://
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-commit
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
___
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
lld
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
___
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 h
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
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
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:
inclu
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 bat
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 th
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_i
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/Symb
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
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
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 call
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 instanti
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:
i
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
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
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::ThreadPoo
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 performa
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
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 L
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 sho
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
sou
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 directl
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 g
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
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 lo
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_conte
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/SystemI
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");
--
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
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
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://revi
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 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 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::
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=-lt
69 matches
Mail list logo
