Page MenuHomePhabricator

scott.smith (Scott Smith)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 20 2017, 11:06 AM (153 w, 4 d)

Recent Activity

May 22 2017

scott.smith added a comment to D32820: Parallelize demangling.

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.

May 22 2017, 12:16 PM

May 16 2017

scott.smith added inline comments to D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions.
May 16 2017, 8:06 PM · Restricted Project
scott.smith added inline comments to D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions.
May 16 2017, 7:53 PM · Restricted Project
scott.smith added inline comments to D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions.
May 16 2017, 7:45 PM · Restricted Project
scott.smith created D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions.
May 16 2017, 10:35 AM · Restricted Project

May 8 2017

scott.smith added a comment to D32823: Remove an expensive lock from Timer.

Can someone please commit this? Thank you!

May 8 2017, 10:13 AM

May 5 2017

scott.smith added reviewers for D32597: Initiate loading of shared libraries in parallel: labath, tberghammer.
May 5 2017, 11:37 AM · Restricted Project
scott.smith added a comment to D32820: Parallelize demangling.

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.

May 5 2017, 10:50 AM
scott.smith added inline comments to D32823: Remove an expensive lock from Timer.
May 5 2017, 9:37 AM
scott.smith updated the diff for D32823: Remove an expensive lock from Timer.

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

May 5 2017, 9:36 AM
scott.smith updated the diff for D32820: Parallelize demangling.

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

May 5 2017, 9:22 AM
scott.smith added inline comments to D32820: Parallelize demangling.
May 5 2017, 7:20 AM

May 4 2017

scott.smith updated the diff for D32820: Parallelize demangling.
  1. git clang-format
  2. new TaskMapOverInt api (begin, end, fn instead of end, batch, fn)
May 4 2017, 8:44 PM
scott.smith updated subscribers of D32597: Initiate loading of shared libraries in parallel.
May 4 2017, 8:37 PM · Restricted Project
scott.smith added a comment to D32757: Add TaskMap for iterating a function over a set of integers.

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!

May 4 2017, 8:31 PM
scott.smith updated the diff for D32757: Add TaskMap for iterating a function over a set of integers.
  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.
May 4 2017, 8:29 PM
scott.smith added a comment to D32757: Add TaskMap for iterating a function over a set of integers.

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.

May 4 2017, 5:54 PM
scott.smith updated the diff for D32823: Remove an expensive lock from Timer.

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

May 4 2017, 2:41 PM
scott.smith added a comment to D32823: Remove an expensive lock from Timer.

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.

May 4 2017, 2:12 PM

May 3 2017

scott.smith added a comment to D32832: Make ConstString creation and access lockfree.

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?

May 3 2017, 3:34 PM
scott.smith added a comment to D32832: Make ConstString creation and access lockfree.

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

May 3 2017, 3:04 PM
scott.smith created D32832: Make ConstString creation and access lockfree.
May 3 2017, 3:02 PM
scott.smith accepted D32826: Move Parallel.h from LLD to LLVM.

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.

May 3 2017, 1:23 PM
scott.smith added a comment to D32826: Move Parallel.h from LLD to LLVM.

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

May 3 2017, 1:20 PM
scott.smith added inline comments to D32597: Initiate loading of shared libraries in parallel.
May 3 2017, 12:39 PM · Restricted Project
scott.smith created D32823: Remove an expensive lock from Timer.
May 3 2017, 12:34 PM
scott.smith added a comment to D32820: Parallelize demangling.

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

May 3 2017, 11:55 AM
scott.smith created D32820: Parallelize demangling.
May 3 2017, 11:54 AM
scott.smith abandoned D32500: Optimize ItaniumDemangle by using an arena allocator.

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.

May 3 2017, 11:49 AM
scott.smith updated the diff for D32597: Initiate loading of shared libraries in parallel.

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.

May 3 2017, 11:39 AM · Restricted Project

May 2 2017

scott.smith added a comment to D32757: Add TaskMap for iterating a function over a set of integers.

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.

May 2 2017, 3:01 PM
scott.smith abandoned D32509: Replace HashString algorithm with xxHash64.

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

May 2 2017, 2:39 PM
scott.smith added a comment to D32757: Add TaskMap for iterating a function over a set of integers.

s/instead of LLVM/instead of LLDB/

May 2 2017, 1:56 PM
scott.smith updated the diff for D32757: Add TaskMap for iterating a function over a set of integers.

change name to TaskRunOverint
remove TaskRunner

May 2 2017, 1:49 PM
scott.smith added a comment to D32757: Add TaskMap for iterating a function over a set of integers.

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.

May 2 2017, 1:48 PM
scott.smith added a comment to D32757: Add TaskMap for iterating a function over a set of integers.

I can make more measurements on this.

May 2 2017, 11:44 AM
scott.smith added a comment to D32757: Add TaskMap for iterating a function over a set of integers.

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.

May 2 2017, 11:35 AM
scott.smith added a reviewer for D32757: Add TaskMap for iterating a function over a set of integers: tberghammer.
May 2 2017, 10:55 AM
scott.smith added a comment to D32757: Add TaskMap for iterating a function over a set of integers.

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

May 2 2017, 10:29 AM
scott.smith created D32757: Add TaskMap for iterating a function over a set of integers.
May 2 2017, 10:28 AM
scott.smith added a comment to D32708: Check for lack of C++ context first when demangling.

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

May 2 2017, 9:26 AM

May 1 2017

scott.smith added a comment to D32708: Check for lack of C++ context first when demangling.

Can someone please commit this? Thank you!

May 1 2017, 11:52 AM
scott.smith added a comment to D32708: Check for lack of C++ context first when demangling.

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.

May 1 2017, 11:16 AM
scott.smith created D32708: Check for lack of C++ context first when demangling.
May 1 2017, 11:11 AM
scott.smith added a comment to D32316: Change UniqueCStringMap to use ConstString as the key.

We can iterate on this.

May 1 2017, 9:42 AM
scott.smith added a comment to D32316: Change UniqueCStringMap to use ConstString as the key.

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

May 1 2017, 9:30 AM
scott.smith added a comment to D32503: Remove unused code related to CPlusPlusLanguage::FindEquivalentNames.

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

May 1 2017, 9:29 AM

Apr 27 2017

scott.smith abandoned D32626: Make the symbol demangling loop order independent.

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

Apr 27 2017, 7:14 PM
scott.smith abandoned D32568: Protect Proces::GetMemoryRegionInfo and ::GetFileLoadAddress with a lock.

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

Apr 27 2017, 5:17 PM
scott.smith retitled D32626: Make the symbol demangling loop order independent from Make the ELF symbol demangling loop order independent to Make the symbol demangling loop order independent.
Apr 27 2017, 4:50 PM
scott.smith added a comment to D32626: Make the symbol demangling loop order independent.

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

Apr 27 2017, 4:50 PM
scott.smith created D32626: Make the symbol demangling loop order independent.
Apr 27 2017, 4:47 PM
scott.smith added a comment to D32598: Prime module caches outside of the module list lock for better parallelism.

Can someone commit this for me? Thanks!

Apr 27 2017, 4:36 PM
scott.smith updated the diff for D32598: Prime module caches outside of the module list lock for better parallelism.

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

Apr 27 2017, 2:34 PM
scott.smith updated the diff for D32598: Prime module caches outside of the module list lock for better parallelism.

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

Apr 27 2017, 1:48 PM
scott.smith updated the diff for D32598: Prime module caches outside of the module list lock for better parallelism.
  1. Rename to preload-symbols / PreloadSymbols()
  2. Modify an existing test to run with and without symbol preloading.
Apr 27 2017, 1:44 PM
scott.smith updated the diff for D32598: Prime module caches outside of the module list lock for better parallelism.

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

Apr 27 2017, 12:24 PM
scott.smith added a comment to D32598: Prime module caches outside of the module list lock for better parallelism.

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.

Apr 27 2017, 11:53 AM
scott.smith added a comment to D32598: Prime module caches outside of the module list lock for better parallelism.

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

Apr 27 2017, 11:49 AM
scott.smith added a comment to D32568: Protect Proces::GetMemoryRegionInfo and ::GetFileLoadAddress with a lock.

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

Apr 27 2017, 11:35 AM
scott.smith added a comment to D32316: Change UniqueCStringMap to use ConstString as the key.

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

Apr 27 2017, 10:43 AM
scott.smith reopened D32509: Replace HashString algorithm with xxHash64.

Reopening as this was reverted.

Apr 27 2017, 10:33 AM
scott.smith added a comment to D32598: Prime module caches outside of the module list lock for better parallelism.

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?

Apr 27 2017, 10:06 AM
scott.smith created D32598: Prime module caches outside of the module list lock for better parallelism.
Apr 27 2017, 10:04 AM
scott.smith added a comment to D32597: Initiate loading of shared libraries in parallel.
Apr 27 2017, 10:00 AM · Restricted Project
scott.smith created D32597: Initiate loading of shared libraries in parallel.
Apr 27 2017, 9:59 AM · Restricted Project
scott.smith added a comment to D32568: Protect Proces::GetMemoryRegionInfo and ::GetFileLoadAddress with a lock.

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.

Apr 27 2017, 9:56 AM
scott.smith added inline comments to D32306: Remove lock from ConstString::GetLength.
Apr 27 2017, 9:53 AM

Apr 26 2017

scott.smith added a comment to D32509: Replace HashString algorithm with xxHash64.
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.

Apr 26 2017, 7:32 PM
scott.smith updated the diff for D32509: Replace HashString algorithm with xxHash64.

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.

Apr 26 2017, 5:33 PM
scott.smith updated the diff for D32306: Remove lock from ConstString::GetLength.

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

Apr 26 2017, 3:59 PM
scott.smith created D32568: Protect Proces::GetMemoryRegionInfo and ::GetFileLoadAddress with a lock.
Apr 26 2017, 3:51 PM
scott.smith abandoned D32565: Make test corrections necessary due to changing llvm::HashString.
Apr 26 2017, 3:41 PM
scott.smith updated the diff for D32509: Replace HashString algorithm with xxHash64.

Updated to include clang test changes.

Apr 26 2017, 3:40 PM
scott.smith created D32565: Make test corrections necessary due to changing llvm::HashString.
Apr 26 2017, 3:29 PM
scott.smith added inline comments to D32306: Remove lock from ConstString::GetLength.
Apr 26 2017, 11:03 AM
scott.smith updated the diff for D32509: Replace HashString algorithm with xxHash64.

Addressed coding style Issue.

Apr 26 2017, 10:10 AM
scott.smith added a comment to D32503: Remove unused code related to CPlusPlusLanguage::FindEquivalentNames.

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

Apr 26 2017, 9:43 AM
scott.smith updated the diff for D32509: Replace HashString algorithm with xxHash64.
Apr 26 2017, 9:39 AM
scott.smith updated the diff for D32509: Replace HashString algorithm with xxHash64.

Updated to reflect the testing changes made by Vedant.

Apr 26 2017, 9:35 AM
scott.smith added a comment to D32503: Remove unused code related to CPlusPlusLanguage::FindEquivalentNames.

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.

Apr 26 2017, 9:16 AM

Apr 25 2017

scott.smith accepted D32512: [gcov] Sort file info before printing it.
Apr 25 2017, 5:23 PM
scott.smith edited reviewers for D32503: Remove unused code related to CPlusPlusLanguage::FindEquivalentNames, added: Restricted Project; removed: lldb-commits.
Apr 25 2017, 4:27 PM
scott.smith removed a reviewer for D32509: Replace HashString algorithm with xxHash64: llvm-commits.
Apr 25 2017, 4:13 PM
scott.smith added a comment to D32509: Replace HashString algorithm with xxHash64.
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").

Apr 25 2017, 4:13 PM
scott.smith updated the diff for D32509: Replace HashString algorithm with xxHash64.

rename param

Apr 25 2017, 4:03 PM
scott.smith created D32509: Replace HashString algorithm with xxHash64.
Apr 25 2017, 3:44 PM
scott.smith added inline comments to D32316: Change UniqueCStringMap to use ConstString as the key.
Apr 25 2017, 2:01 PM
scott.smith updated the diff for D32316: Change UniqueCStringMap to use ConstString as the key.

address review comments

Apr 25 2017, 2:00 PM
scott.smith added inline comments to D32500: Optimize ItaniumDemangle by using an arena allocator.
Apr 25 2017, 1:22 PM
scott.smith created D32503: Remove unused code related to CPlusPlusLanguage::FindEquivalentNames.
Apr 25 2017, 1:01 PM
scott.smith created D32500: Optimize ItaniumDemangle by using an arena allocator.
Apr 25 2017, 11:49 AM

Apr 24 2017

scott.smith added a comment to D32306: Remove lock from ConstString::GetLength.

Looks good, thank you.

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

Apr 24 2017, 4:42 PM
scott.smith added a comment to D32316: Change UniqueCStringMap to use ConstString as the key.

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.

Apr 24 2017, 12:12 PM

Apr 20 2017

scott.smith added a reviewer for D32316: Change UniqueCStringMap to use ConstString as the key: clayborg.
Apr 20 2017, 5:20 PM
scott.smith updated the summary of D32316: Change UniqueCStringMap to use ConstString as the key.
Apr 20 2017, 3:15 PM
scott.smith created D32316: Change UniqueCStringMap to use ConstString as the key.
Apr 20 2017, 3:14 PM
scott.smith created D32306: Remove lock from ConstString::GetLength.
Apr 20 2017, 11:44 AM