In 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.

Can someone please commit this? Thank you!

In 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.

remove same-name-different-site support from Timer::Category

sea of red and green
Be explicit about what the lambda has access to (and make the inputs const) to prevent concurrency bugs.

1. git clang-format
2. new TaskMapOverInt api (begin, end, fn instead of end, batch, fn)

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!

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.

In 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.

updated per review comments.
added a test to fix the Jurassic Park problem.

In 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.

In 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?

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).

In D32826#745258, @zturner wrote:

I think it woudl be better if improvements were made independently of a code move. Because if you combine improvements and code moves, the improvements kind of get "lost" in the history.

Is the point just to move code, or to field improvements (in this code review)?

This commit uses TaskMapOverInt from change D32757, which hasn't landed yet.

With added parallelism to lldb, this change no longer has an impact, and I sense an uphill battle trying to get this into libcxx, and then llvm.

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.

In 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.

I'll get much better performance by just replacing lldb::ConstString with a lock free hashtable utilizing its own hash function.

In D32757#743796, @zturner wrote:

s/instead of LLVM/instead of LLDB/

change name to TaskRunOverint
remove TaskRunner

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.

I can make more measurements on this.

In D32757#743657, @clayborg wrote:

In 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.

IMO, this is a simpler interface than TaskRunner. Also, after this, there are no users of TaskRunner left. Should I just delete them?

In D32708#743161, @labath wrote:

I am having trouble applying this. Do you need to rebase or something?

Can someone please commit this? Thank you!

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.

In D32316#742286, @clayborg wrote:

We can iterate on this.

In D32316#739699, @scott.smith wrote:

Can I get a re-review on this? Thanks.

In D32503#738274, @scott.smith wrote:

Can someone please commit this for me? I don't have access. Thank you!

Turns out I'm planning on making more drastic changes to this loop, so there's no point in reviewing this step.

Further testing shows this is not necessary. I'll abandon it.

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 :-)

Can someone commit this for me? Thanks!

Add test to print expression (calls func, hence resolves symbols). Also better parameterize the common test to reduce code duplication.

Fix default param to setup_common so that we actually test both paths.

1. Rename to preload-symbols / PreloadSymbols()
2. Modify an existing test to run with and without symbol preloading.

Added setting. Verified that:
settings set target.cache-priming off
works as expected.

In 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.

In D32598#739779, @clayborg wrote:

Making an empty main program and saying I see no difference is not enough testing to enable this.

In D32568#739723, @labath wrote:

All your other changes are client-side, so still think this will not matter, but I'll take a look. :)

Can I get a re-review on this? Thanks.

Reopening as this was reverted.

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?

In 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.

In D32509#738971, @vsk wrote:

Is it necessary to have a mac to run the mach-o tests? At any rate, changing the 'CHECK'/'DCHECK' lines to 'CHECK-DAG'/'DCHECK-DAG' should do the trick.

Fixed clang-tools-extra build issues, but doesn't fix Mac lld issues (http://lab.llvm.org:8011/builders/lld-x86_64-darwin13/builds/7423/steps/test_lld/logs/stdio) as I do not have access to a Mac. Can someone look into it? It's probably just an identifier ordering issue.

Use StringMapEntry::GetStringMapEntryFromKeyData instead of ConstString's version.

Updated to include clang test changes.

Addressed coding style Issue.

Can someone please commit this for me? I don't have access. Thank you!

Updated to reflect the testing changes made by Vedant.

In 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.

In D32509#737488, @vsk wrote:

Thanks for working on this! In the future, please set llvm-commits as a subscriber on differential revisions, not as a reviewer. Also, please upload patches with context (e.g "git diff -U1000").

rename param

address review comments

In D32306#733316, @labath wrote:

Looks good, thank you.

Out of curiosity, have you observed any performance improvements resulting from this?

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.