User Details
- User Since
- Apr 20 2017, 11:06 AM (301 w, 3 d)
May 22 2017
May 16 2017
May 8 2017
Can someone please commit this? Thank you!
May 5 2017
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.
May 4 2017
- git clang-format
- 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!
- Change the API to more closely match parallel_for (begin, end, fn) instead of (end, batch, fn).
- Fix unit test. I didn't realize I had to run check-lldb-unit separately from dotest (oops).
- Fix formatting via git-clang-format.
updated per review comments.
added a test to fix the Jurassic Park problem.
May 3 2017
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).
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.
May 2 2017
I'll get much better performance by just replacing lldb::ConstString with a lock free hashtable utilizing its own hash function.
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.
IMO, this is a simpler interface than TaskRunner. Also, after this, there are no users of TaskRunner left. Should I just delete them?
May 1 2017
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.
Apr 27 2017
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.
- Rename to preload-symbols / PreloadSymbols()
- Modify an existing test to run with and without symbol preloading.
Added setting. Verified that:
settings set target.cache-priming off
works as expected.
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?
Apr 26 2017
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.
Apr 25 2017
rename param
address review comments
Apr 24 2017
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.