Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
167893009:
[Android] Add fast ELF Symbolizer to pylib. (Closed)
|
Created:
6 years, 10 months ago by Primiano Tucci (use gerrit) Modified:
6 years, 10 months ago CC:
chromium-reviews, Dai Mikurube (NOT FULLTIME) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd fast ELF Symbolizer to memory_inspector.
This CL introduces a multiprocess, pipelined and asynchronous ELF
symbolizer (based on addr2line) which gives honor to a bulkly workstation
when symbolizing large batches of symbols.
BUG=340294, 339059
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252963
Patch Set 1 : #Patch Set 2 : Performance tuning #
Total comments: 1
Patch Set 3 : Focusing only on the inner ELFSymbolizer #
Total comments: 20
Patch Set 4 : Move to pylib + pliard nits #
Total comments: 23
Patch Set 5 : bulach@ nits #
Total comments: 1
Patch Set 6 : more bulach@ nits #Patch Set 7 : Remaining nits on mock_addr2line #Patch Set 8 : Add inlines support + tests #
Messages
Total messages: 30 (0 generated)
Because symbolization is never fast enough! ;-) https://codereview.chromium.org/167893009/diff/50001/tools/memory_inspector/m... File tools/memory_inspector/memory_inspector/backends/android/android_backend_unittest.py (left): https://codereview.chromium.org/167893009/diff/50001/tools/memory_inspector/m... tools/memory_inspector/memory_inspector/backends/android/android_backend_unittest.py:5: """Unittests for the backends.android modules.""" This was just missing after the last test refactoring. It's pretty tautological given that the name of this file is now "android_backend_unittest".
Ah, +andrewhayden. IIRC you did something similar for the binary size tool (or maybe I'm just terribly confused)? In any case, if you have any thoughts, doubts, extra ideas, they're more than welcome.
Yeah, this is the review for the binary size tool: https://codereview.chromium.org/119083006/ What you have here looks great overall, and looks like it will save me a lot of work! There are several features I'd add first, but that can be done after checkin (sorting and deduplication of addresses prior to processing, disambiguation of paths for binaries that are missing absolute path data, processing limits for addr2line). I'll have a more detailed look.
On 2014/02/18 08:48:03, Andrew Hayden wrote: > Yeah, this is the review for the binary size tool: > https://codereview.chromium.org/119083006/ Ah nice! > There are several features I'd add first, but that can be done after checkin > sorting and deduplication of addresses prior to processing, Hmm not really sure this should be a responsibility of the symbolizer itself, or of the guy which feeds the symbolizer. The symbolizer is designed to be streaming (i.e. asynchronous). Is the only way to leverage pipelining. Sorting will be pretty hard to achieve in a streaming architecture :) > disambiguation of paths for binaries that are missing absolute path data Hmm more details? seems interesting. If this is what I'm thinking, this is already looking up libs by name, not by abs path (see _FindLibByName). > processing limits for addr2line). Hmm what do you mean? You mean watching for addr2line OOM crashes? In WaitForNextSymbolInQueue this already tolerates addr2line crashing and hanging. In fact you can killall -9 addr2line or killall -STOP addr2line as many times as you feel like during the symbolization and that will keep working ;) (modulo some delay to detect addr2line timeouts). > I'll have a more detailed look. Sure, Thanks
After some discussion with pliard@ (thanks very much Phelippe) I'm going some refactoring of this changes. The problem that this change is going to solve (parallel symbolization of multiple libraries) is a bit complicated per its nature, and solving that entirely in a single module makes it a bit difficult to reason about. After some offline discussion we agreed that this should be conceptually divided into two layers (effectively two python classes or modules) A Library symbolizer: which solves the problem of batch-symbolizing a single file. i.e. sharding symbolization for a single lib.so among a pool of N addr2line instances. A "Project symbolizer": kind of an outer layer which solves the problem of symbolizing several libraries, and coordinates many library-symbolizer pools. Will come back to this CL with an updated patchset.
sorry, still fighting the other fires, but just in case: have you seen symbol.py, right? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/androi... there's a CallAddr2LineForSet that does the "underlying layer" you mention, and it's memoized.. we have then higher up layers, say, tombstones, that does some further parallelization: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/tomb... I hope you'll be able to get around with reusing that and just implementing the higher up layer, i.e., the parallelization per-project?
On 2014/02/18 13:59:48, bulach wrote: > sorry, still fighting the other fires, but just in case: > have you seen symbol.py, right? Don't worry I know how busy the Bulach Chromium Consulting is in this period :) > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/androi... > there's a CallAddr2LineForSet that does the "underlying layer" you mention, and > it's memoized.. Yeah I've seen it, unfortunately, from what I can see: - It is not pipelined. Conversely to what you might expect from something which takes in input a set of addresses, it pushes them one by one into stdin and wait for addr2line to respond (on each of them) before pushing the next one. - Is is not parallel, and I hate seeing CPU cores idling when there is work to do. :) Also, addr2line works pretty well in parallel when several instances work on the same file at the same time, as they keep hitting the pagecache, and most of the processes become mostly CPU bound. - Memoizing (or caching in general) symbols in the python code scares me a bit when we're taking about a lot of symbols. Also from what I remember (and I think andrewhyden@ made some similar investigations) addr2line is pretty good on caching itself and there's no substantial benefit in adding a caching layer in the client side (provide we pipe fast enough). I prefer much more spawning instances of addr2line, let them die for OOM, and respawn them, rather than seeing my poor python script die. > we have then higher up layers, say, tombstones, that does some further > parallelization: > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/tomb... I am not very familiar with the tombstones themselves and their use cases. I've just noticed that the only way to go really fast is pipelining and handling the symbols asynchronously as they come (and throwing away the memory as soon as it has been used). I don't like a lot the idea of collecting gigantic collections of information per stage and then doing all the processing in bulk. Essentially the problem this class is going to resolve is making the symbolization happen in streaming. > I hope you'll be able to get around with reusing that and just implementing the > higher up layer, i.e., the parallelization per-project? Eventually, if I get more bandwidth, wouldn't it make more sense backporting this symbolizer to the rest of chromium (the files you pointed out)?
On 2014/02/18 14:14:47, Primiano Tucci wrote: > On 2014/02/18 13:59:48, bulach wrote: > > sorry, still fighting the other fires, but just in case: > > have you seen symbol.py, right? > Don't worry I know how busy the Bulach Chromium Consulting is in this period :) > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/androi... > > there's a CallAddr2LineForSet that does the "underlying layer" you mention, > and > > it's memoized.. > > Yeah I've seen it, unfortunately, from what I can see: > - It is not pipelined. Conversely to what you might expect from something which > takes in input a set of addresses, it pushes them one by one into stdin and wait > for addr2line to respond (on each of them) before pushing the next one. > - Is is not parallel, and I hate seeing CPU cores idling when there is work to > do. :) Also, addr2line works pretty well in parallel when several instances work > on the same file at the same time, as they keep hitting the pagecache, and most > of the processes become mostly CPU bound. > - Memoizing (or caching in general) symbols in the python code scares me a bit > when we're taking about a lot of symbols. Also from what I remember (and I think > andrewhyden@ made some similar investigations) addr2line is pretty good on > caching itself and there's no substantial benefit in adding a caching layer in > the client side (provide we pipe fast enough). I prefer much more spawning > instances of addr2line, let them die for OOM, and respawn them, rather than > seeing my poor python script die. > > > we have then higher up layers, say, tombstones, that does some further > > parallelization: > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/tomb... > > I am not very familiar with the tombstones themselves and their use cases. I've > just noticed that the only way to go really fast is pipelining and handling the > symbols asynchronously as they come (and throwing away the memory as soon as it > has been used). I don't like a lot the idea of collecting gigantic collections > of information per stage and then doing all the processing in bulk. > Essentially the problem this class is going to resolve is making the > symbolization happen in streaming. so symbol.py does some of this streaming, i.e., it puts data into stdin into addr2line and reads its output.. it looks doable to do the improvements you mentioned in symbol.py? > > > I hope you'll be able to get around with reusing that and just implementing > the > > higher up layer, i.e., the parallelization per-project? > > Eventually, if I get more bandwidth, wouldn't it make more sense backporting > this symbolizer to the rest of chromium (the files you pointed out)?
So, just to keep things more clear, I splitted the two layers. This CL contains only the inner (i.e. single-elf symbolizer) layer. The outer (project-level) will come in another CL. Let's discuss the inner one here. > so symbol.py does some of this streaming, i.e., it puts data into stdin into > addr2line and reads its output.. It is true but... The real problem is not streaming. The point is doing that asyncrhonously. In the very essence, if you "unroll the loop", symbol.py does something like this: => write addr 1 to stdin <= read symbol 1 from stdout (blocks and does nothing until symbol 1 is outputed from addr2line) => write addr 2 to stdin <= read symbol 1 from stdout => write addr 3 to stdin <= read symbol 3 from stdout => write addr 4 to stdin <= read symbol 4 from stdout Instead, my symbolizer here does async pipelining like this (without taking into account any multiprocessing, i.e. with max_parallelism=1) => write addr 1 to stdin => write addr 2 to stdin <? "poll" stdout, did produce any out? No: damn slow addr2line, let's keep going => write addr 3 to stdin <? poll stdout. Oh wow, this time it produced something <= read addr1, addr2 from stdout => write addr 4 to std <= read addr3 from stdout In essence symbol.py is not leveraging at all the piplined architecture of addr2line. In more concrete (i.e. benchmark) terms it translate into this: #10000 symbols from libwebviewchromium.so using symbol.py $ time python benchmark_symbol.py real 2m23.998s user 0m0.190s sys 0m0.030s # same using my symbolizer in single-process-mode (i.e. max_parallelism=1) $ time python benchmark_elfsymbolizer_1.py real 1m23.304s #~ 1 minute faster, 1.72x speedup just pipelining. The important is not going fast. The important is hiding the latencies :) user 0m1.900s sys 0m0.430s # same using my symbolizer in multi-process-mode (i.e. max_parallelism=6) $ time python benchmark_elfsymbolizer_6.py real 0m18.215s # 7.9x speedup using pipelining and multiprocessing (well, if we can also go fast....) user 0m1.350s sys 0m0.250s > it looks doable to do the improvements you mentioned in symbol.py? Hmm, the point is that symbol.py has a lot of assumptions in it I don't need / I need to change, such as: - knows where and how to find addr2line - knows where and how to find symbol files - is designed to exchange "collections" of stuff (i.e. give me a collection of addresses, wait some time, and get in return a collection of symbols) Also is my understanding that there are various tools out there which rely on these assumptions and want to keep them such. I'm always a bit scared on touching code on which many other places (bots, other scripts) currently rely on. (But i'm not saying this is a reason for not changing it, see below) As I explained above, my elf_symbolizer class has a slightly different design, that relies on the fact that the client is able to work asynchronously. It is not just a "faster version" of symbol.py. Just does the some thing in a different (i.e. asynchronous) way. Which, on the other side requires that the other participants (i.e. the caller) is willing to work asynchronously as well. The problem is now that is easy to adapt something that has a synchronous interface (i.e. symbol.py) to use internally something that is asynchronous (i.e. elf_symbolizer.py). The other way round is pretty hard. What I'm trying to say here is that I can think of moving this elf_symbolizer.py somewhere else and tell symbol.py to use this elf_symbolizer in its internals (so the outer CallAddr2LineForSet interface remains unchanged). The other way round (refactoring symbol.py to make it usable by current clients and myself) sounds a bit of a pain for me. If there is enough interest in the first approach I can do that. it will take me a bit of time but I think I can do without breaking the world (the last famous words) .
On 2014/02/18 15:48:57, Primiano Tucci wrote:
> So, just to keep things more clear, I splitted the two layers.
> This CL contains only the inner (i.e. single-elf symbolizer) layer.
> The outer (project-level) will come in another CL.
>
> Let's discuss the inner one here.
>
> > so symbol.py does some of this streaming, i.e., it puts data into stdin into
> > addr2line and reads its output..
> It is true but...
> The real problem is not streaming. The point is doing that asyncrhonously.
> In the very essence, if you "unroll the loop", symbol.py does something like
> this:
>
> => write addr 1 to stdin
> <= read symbol 1 from stdout (blocks and does nothing until symbol 1 is
outputed
> from addr2line)
> => write addr 2 to stdin
> <= read symbol 1 from stdout
> => write addr 3 to stdin
> <= read symbol 3 from stdout
> => write addr 4 to stdin
> <= read symbol 4 from stdout
>
> Instead, my symbolizer here does async pipelining like this (without taking
into
> account any multiprocessing, i.e. with max_parallelism=1)
> => write addr 1 to stdin
> => write addr 2 to stdin
> <? "poll" stdout, did produce any out? No: damn slow addr2line, let's keep
> going
> => write addr 3 to stdin
> <? poll stdout. Oh wow, this time it produced something
> <= read addr1, addr2 from stdout
> => write addr 4 to std
> <= read addr3 from stdout
>
> In essence symbol.py is not leveraging at all the piplined architecture of
> addr2line.
> In more concrete (i.e. benchmark) terms it translate into this:
>
> #10000 symbols from libwebviewchromium.so using symbol.py
> $ time python benchmark_symbol.py
> real 2m23.998s
> user 0m0.190s
> sys 0m0.030s
>
> # same using my symbolizer in single-process-mode (i.e. max_parallelism=1)
> $ time python benchmark_elfsymbolizer_1.py
> real 1m23.304s #~ 1 minute faster, 1.72x speedup just pipelining. The
> important is not going fast. The important is hiding the latencies :)
> user 0m1.900s
> sys 0m0.430s
>
> # same using my symbolizer in multi-process-mode (i.e. max_parallelism=6)
> $ time python benchmark_elfsymbolizer_6.py
> real 0m18.215s # 7.9x speedup using pipelining and multiprocessing (well,
if
> we can also go fast....)
> user 0m1.350s
> sys 0m0.250s
>
>
> > it looks doable to do the improvements you mentioned in symbol.py?
> Hmm, the point is that symbol.py has a lot of assumptions in it I don't need /
I
> need to change, such as:
> - knows where and how to find addr2line
> - knows where and how to find symbol files
> - is designed to exchange "collections" of stuff (i.e. give me a collection of
> addresses, wait some time, and get in return a collection of symbols)
>
> Also is my understanding that there are various tools out there which rely on
> these assumptions and want to keep them such. I'm always a bit scared on
> touching code on which many other places (bots, other scripts) currently rely
> on. (But i'm not saying this is a reason for not changing it, see below)
>
> As I explained above, my elf_symbolizer class has a slightly different design,
> that relies on the fact that the client is able to work asynchronously. It is
> not just a "faster version" of symbol.py. Just does the some thing in a
> different (i.e. asynchronous) way. Which, on the other side requires that the
> other participants (i.e. the caller) is willing to work asynchronously as
well.
> The problem is now that is easy to adapt something that has a synchronous
> interface (i.e. symbol.py) to use internally something that is asynchronous
> (i.e. elf_symbolizer.py). The other way round is pretty hard.
> What I'm trying to say here is that I can think of moving this
elf_symbolizer.py
> somewhere else and tell symbol.py to use this elf_symbolizer in its internals
> (so the outer CallAddr2LineForSet interface remains unchanged).
> The other way round (refactoring symbol.py to make it usable by current
clients
> and myself) sounds a bit of a pain for me.
> If there is enough interest in the first approach I can do that. it will take
me
> a bit of time but I think I can do without breaking the world (the last famous
> words) .
sorry, I wasn't clear... :) I'm not discussing the need for a new API, I'm just
saying that we should put at a "lower level" that can then be reused by various
other tools!
I *fully* agree the pipelined design looks better and faster.
Specifically, on symbol.py, there is:
CallAddr2LineForSet(lib, unique_addrs):
traverses input:
calls addr2line
returns a dict {addr: [(symbol, file:line)]}
It looks to me that we could:
1) add a new public API _in that_ file for the asynchronous / multiprocess you
mentioned...
2) change the CallAddr2LineForSet to use the new API: it takes a lib and a set
of addresses, so it can totally benefit from using the new async / multiprocess
API *even* if it hides that fact, right?
even if it means:
CallAddr2LineForSet(lib, unique_addrs):
traverses input:
append(ShovesIntoNewApi())
returns a dict {addr: [(symbol, file:line)]}
...orthogonal:
3) on a case-by-case basis, we could then change the other (many) callers to use
the new implementation?
> It looks to me that we could:
> 1) add a new public API _in that_ file for the asynchronous / multiprocess you
> mentioned...
> 2) change the CallAddr2LineForSet to use the new API: it takes a lib and a set
> of addresses, so it can totally benefit from using the new async /
multiprocess
> API *even* if it hides that fact, right?
Ohh right, now I get it and... yup, I think we're perfectly aligned.
Talking now more concretely about code, I think at this point elf_symbolizer.py
(which I refactored in the latest patchset in this CL) is the internal API we're
talking about :)
Clearly the current proposed location is wrong, but if you briefly look at the
code you'll see that it is an independent module (no references to
memory_inspector at all) that just takes care of symbolizing one elf file.
In the very essence, the overall legacy plumbing (i.e. to keep the same outer
interface of symbol.py) would look pretty much like this:
from somewhere import elf_symbolizer
CallAddr2LineForSet(lib, unique_addrs):
symbols = []
callback = lambda sym_info: symbols += [sym_info] #Eventually some small
boilerplate to match sym_info to the return format of CallAddr2LineForSet
ELFSymbolizer s = ELFSymbolizer(lib, callback)
for addr in unique_addrs:
s.RequestSymbol(addr)
s.Join()
return symbols
My question now is: where do we plumb this?
- In the /third_party/android_platform/ ?
I don't really like this, first of all because it's under third_party (also,
it not really android_platform specific).
Secondly because that path is not really py-lib friendly. I had a lot of fun
trying to "import scripts": it works until that one is very early in the path,
and I don't really like stuff to be dependent on their position in the
(PYTHON)PATH
Dumb question: do we have a official/pseudo-official chromium python library
directory (i.e. with directory structure which are py-lib friendly)? I mean
something for common API/modules like this which are going to be shared by
several tools?
> ...orthogonal:
> 3) on a case-by-case basis, we could then change the other (many) callers to
use
> the new implementation?
Right! But I have no idea at the moment about which are these cases.
On 2014/02/18 16:29:28, Primiano Tucci wrote: > > It looks to me that we could: > > 1) add a new public API _in that_ file for the asynchronous / multiprocess you > > mentioned... > > 2) change the CallAddr2LineForSet to use the new API: it takes a lib and a set > > of addresses, so it can totally benefit from using the new async / > multiprocess > > API *even* if it hides that fact, right? > > Ohh right, now I get it and... yup, I think we're perfectly aligned. > Talking now more concretely about code, I think at this point elf_symbolizer.py > (which I refactored in the latest patchset in this CL) is the internal API we're > talking about :) > Clearly the current proposed location is wrong, but if you briefly look at the > code you'll see that it is an independent module (no references to > memory_inspector at all) that just takes care of symbolizing one elf file. > > In the very essence, the overall legacy plumbing (i.e. to keep the same outer > interface of symbol.py) would look pretty much like this: > > from somewhere import elf_symbolizer > CallAddr2LineForSet(lib, unique_addrs): > symbols = [] > callback = lambda sym_info: symbols += [sym_info] #Eventually some small > boilerplate to match sym_info to the return format of CallAddr2LineForSet > ELFSymbolizer s = ELFSymbolizer(lib, callback) > for addr in unique_addrs: > s.RequestSymbol(addr) > s.Join() > return symbols that's right! and this will "stack building blocks together" rather than creating yet-another-independent module that would eventually evolve in yet-another-way ;) also: the buildbots do already run this "symbol.py" on every single build, so even a small improvement would be a positive gain. > > > My question now is: where do we plumb this? > - In the /third_party/android_platform/ ? > I don't really like this, first of all because it's under third_party (also, > it not really android_platform specific). > Secondly because that path is not really py-lib friendly. I had a lot of fun > trying to "import scripts": it works until that one is very early in the path, > and I don't really like stuff to be dependent on their position in the > (PYTHON)PATH > > Dumb question: do we have a official/pseudo-official chromium python library > directory (i.e. with directory structure which are py-lib friendly)? I mean > something for common API/modules like this which are going to be shared by > several tools? Yeah, due to some of these bizareness, third_party/android_platform is actually importing build/android/pylib I think creating a directory build/android/pylib/symbols/ would be fine by me! > > > > ...orthogonal: > > 3) on a case-by-case basis, we could then change the other (many) callers to > use > > the new implementation? > Right! But I have no idea at the moment about which are these cases. I know a few (guess who added all of them..). I'm happy to help with that.. ;) again, it's separate, as long as the api stays the same, and at least the current most basic block uses it, we're good! (i.e., all these other scripts eventually go down to symbol.py).
Thanks Primiano! This looks mostly good to me, I only have minor comments/naming suggestions. I will approve tomorrow :) I wish we could use a single queue and have all the addr2line processes have their stdin backed by a shared pipe but we would then run into some synchronization issues. Addr2line would have to use a message queue to make this work or a semaphore but we definitely don't want to make any (intrusive) change in addr2line :) So FWIW the approach you took (having one pipe for each addr2line process) is definitely needed/the best IMO :) https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... File tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py (right): https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:61: |max_backlog| bound, a new addr2line instance is kicked in. I wonder if s/backlog/request_queue/g could improve clarity overall :) |max_backlog| would then be |max_queue_size|. |address_queue| would be even clearer IMO although we are not pushing raw addresses to the queue but rather address contexts. https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:67: other modules in this project), to allow easy reuse in external projects. Thanks for the nice comment! https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:73: max_parallelism=None, addr2line_timeout=30, max_backlog=50): Nit: maybe s/max_parallelism/max_jobs_count/g would be clearer. Up to you though :) https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:103: self._affinity_cache = {} # Map hash(addr) -> _Addr2Line instance. Just out of curiosity, did you measure the speed impact of this affinity cache? I'm completely fine with it if you didn't since it's only adding 4 lines modulo the big comment though :) Is this used to leverage some internal caching performed in addr2line itself? (as opposed to the pagecache which can be hit no matter which addr2line process we use) https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:199: self.backlog = 0 Nit: |pending_syms_size| would be clearer here or |request_queue_size| if |_pending_syms| is renamed to |_request_queue|. https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:235: if self._proc.poll(): The fact that they called it poll() here (without even having it blocking) is really misleading :) I was expecting this to block until there is data ready to be read on the pipe. https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:243: (line1, line2) = self._out_queue.get(block=True, timeout=0.25) Is this call the one possibly throwing the Queue.Empty exception? In that case you can remove the '1 s.' from the comment below line 248 and maybe make it slightly clearer that the exception was thrown by this get(). https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:310: self.Terminate() Should Terminate() also reset the self._proc pointer or is the boolean test above on line 309 checking "more" than whether the pointer is NULL or not? This doesn't matter much in practice though since we immediately overwrite |self._proc| below in line 320.
> Yeah, due to some of these bizareness, third_party/android_platform is actually > importing build/android/pylib > I think creating a directory build/android/pylib/symbols/ would be fine by me! If I say that I dream of a structured directory where we put all the python chromium libs (not the tools), instead of spreading them among several projects which cross-import each other and do magic mangling of the PYTHONPATH in order to get them right... am I dreaming too much? :D Something like: $CHROME_SRC/pylib/chromium/android/adb/ $CHROME_SRC/pylib/chromium/memory/posix/ $CHROME_SRC/pylib/chromium/memory/windows/ $CHROME_SRC/pylib/chromium/symbolization/posix/ $CHROME_SRC/pylib/chromium/perf/linux/ $CHROME_SRC/pylib/chromium/perf/android/ Some kind of civilized world in which everybody adds only $CHROME_SRC/pylib to the PYTHONPATH and everybody uses a uniform namespace like import chromium.android.adb import chromium.memory.windows..... Going back to this CL, there nothing practically bad with using build/android/pylib/ (also I can't think of a better alternative at the moment). it's just that conceptually this has nothing to do with the "build" (eventually that's one of the clients) and has nothing to do strictly with android (works also with linux). I feel pretty sick every time I have to include a "test_runner" just to use adb(.adb.adb) and similar :/ Maybe I should just turn my OCD off? :) or .... > I know a few (guess who added all of them..). > I'm happy to help with that.. ;) > again, it's separate, as long as the api stays the same, and at least the Yeah right! That should be the sense of an api :) (Even though, with very little effort, I can remember some other cases in which every time I have to make a one-line-change I have to change some random API ;-) )
>I wish we could use a single queue and have all the addr2line processes have >their stdin backed by a shared pipe but we would then run into some >synchronization issues. >Addr2line would have to use a message queue to make this work or a semaphore but >we definitely don't want to make any (intrusive) change in addr2line :) Essentially, what you wish is that addr2line was "parallel inside", in which case we wouldn't have needed most of the complexity here. Essentially the "message queue" here is moved to the client, who has to perform the asynchronous calls. We here do just the dispatching to the addr2line instances (modulo the backlog/request_queues which is a very tiny queue to keep each addr2line "pumped") >So FWIW the approach you took (having one pipe for each addr2line process) is definitely needed/the best IMO :) Right, hacking GNU binutils (to make them multithreaded) scares me quite a bit ^__^. Also, given its aggressive caching, I prefer to treat addr2line as something which can die at any time and managing it from the outside (like this python module), rather than moving all the complexity inside it. Many thanks for the comment, I'll address them tomorrow. https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... File tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py (right): https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:61: |max_backlog| bound, a new addr2line instance is kicked in. On 2014/02/18 18:12:34, Philippe wrote: > I wonder if s/backlog/request_queue/g could improve clarity overall :) > |max_backlog| would then be |max_queue_size|. Makes sense to me. I never liked "backlog" too much, too generic. > |address_queue| would be even clearer IMO although we are not pushing raw > addresses to the queue but rather address contexts. The raw vs. context addr doesn't worry me too much (any sensible person using a symbolizer would expect to provide addresses which are relative to the eld file, not to the runtime load point). It's just that address queue is IMHO a bit harder to get at a first glance (doesn't really tell where these addresses are flowing from/to). While request queue gives a better impression of where that data will end up. https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:67: other modules in this project), to allow easy reuse in external projects. On 2014/02/18 18:12:34, Philippe wrote: > Thanks for the nice comment! np :) https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:73: max_parallelism=None, addr2line_timeout=30, max_backlog=50): On 2014/02/18 18:12:34, Philippe wrote: > Nit: maybe s/max_parallelism/max_jobs_count/g would be clearer. Up to you though > :) What about max_concurrent_jobs? https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:103: self._affinity_cache = {} # Map hash(addr) -> _Addr2Line instance. On 2014/02/18 18:12:34, Philippe wrote: > Just out of curiosity, did you measure the speed impact of this affinity cache? > I'm completely fine with it if you didn't since it's only adding 4 lines modulo > the big comment though :) > > Is this used to leverage some internal caching performed in addr2line itself? > (as opposed to the pagecache which can be hit no matter which addr2line process > we use) That's a very good point. You trigger my "counting" obsession :) I did a benchmark over a fixed number of 10000 addresses varying the paralellism and the "repetition ratio" of identical addresses. Results here: https://docs.google.com/spreadsheet/ccc?key=0AnKuDeqD-lVJdGh3RFJBdkJPNWVxaXU0... The (orrifying) code is here: http://paste/4844451721641984 It looks like that the speedup of the cache is not that great win. Most of the times the cached variant is 5-10% slower than the non-cache one. My impression is that the improvement is not really distinguishable from the noise. So even if they're just 4 line I'm tempted to remove them. https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:199: self.backlog = 0 On 2014/02/18 18:12:34, Philippe wrote: > Nit: |pending_syms_size| would be clearer here or |request_queue_size| if > |_pending_syms| is renamed to |_request_queue|. Agree! https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:235: if self._proc.poll(): On 2014/02/18 18:12:34, Philippe wrote: > The fact that they called it poll() here (without even having it blocking) is > really misleading :) I was expecting this to block until there is data ready to > be read on the pipe. Ha! I have to cite my university advisor "Don't use names which are too explicative, otherwise you take the risk that other people will understand you" :) https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:243: (line1, line2) = self._out_queue.get(block=True, timeout=0.25) On 2014/02/18 18:12:34, Philippe wrote: > Is this call the one possibly throwing the Queue.Empty exception? In that case It looks so. > you can remove the '1 s.' from the comment below line 248 and maybe make it > slightly clearer that the exception was thrown by this get(). Hmm, I'm missing this. I need to timeout after 0.25 s (I forgot to update the comment below) because I want to promptly detect crashes. If I don't timeout and the process crashes, this code would keep waiting undefinitely on the queue. https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:310: self.Terminate() On 2014/02/18 18:12:34, Philippe wrote: > Should Terminate() also reset the self._proc pointer Ah right! Makes sense. Nice catch! > or is the boolean test > above on line 309 checking "more" than whether the pointer is NULL or not? It shouldn't have functional implications though since terminate is "guarded" by try/catch. But makes sense to make it consisrtent. Also as you say, in the other cases the process is immediately overwritten. > This doesn't matter much in practice though since we immediately overwrite > |self._proc| below in line 320. Yeah right, but you know... OCD :)
https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... File tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py (right): https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:73: max_parallelism=None, addr2line_timeout=30, max_backlog=50): On 2014/02/18 20:16:12, Primiano Tucci wrote: > On 2014/02/18 18:12:34, Philippe wrote: > > Nit: maybe s/max_parallelism/max_jobs_count/g would be clearer. Up to you > though > > :) > > What about max_concurrent_jobs? Yeah, even better! https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:103: self._affinity_cache = {} # Map hash(addr) -> _Addr2Line instance. On 2014/02/18 20:16:12, Primiano Tucci wrote: > On 2014/02/18 18:12:34, Philippe wrote: > > Just out of curiosity, did you measure the speed impact of this affinity > cache? > > I'm completely fine with it if you didn't since it's only adding 4 lines > modulo > > the big comment though :) > > > > Is this used to leverage some internal caching performed in addr2line itself? > > (as opposed to the pagecache which can be hit no matter which addr2line > process > > we use) > That's a very good point. You trigger my "counting" obsession :) > I did a benchmark over a fixed number of 10000 addresses varying the paralellism > and the "repetition ratio" of identical addresses. > Results here: > https://docs.google.com/spreadsheet/ccc?key=0AnKuDeqD-lVJdGh3RFJBdkJPNWVxaXU0... > The (orrifying) code is here: http://paste/4844451721641984 > It looks like that the speedup of the cache is not that great win. Most of the > times the cached variant is 5-10% slower than the non-cache one. > My impression is that the improvement is not really distinguishable from the > noise. So even if they're just 4 line I'm tempted to remove them. Agreed. Thanks for the benchmark! https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:235: if self._proc.poll(): On 2014/02/18 20:16:12, Primiano Tucci wrote: > On 2014/02/18 18:12:34, Philippe wrote: > > The fact that they called it poll() here (without even having it blocking) is > > really misleading :) I was expecting this to block until there is data ready > to > > be read on the pipe. > Ha! I have to cite my university advisor "Don't use names which are too > explicative, otherwise you take the risk that other people will understand you" > :) > Haha nice :) https://codereview.chromium.org/167893009/diff/190001/tools/memory_inspector/... tools/memory_inspector/memory_inspector/backends/android/elf_symbolizer.py:243: (line1, line2) = self._out_queue.get(block=True, timeout=0.25) On 2014/02/18 20:16:12, Primiano Tucci wrote: > On 2014/02/18 18:12:34, Philippe wrote: > > Is this call the one possibly throwing the Queue.Empty exception? In that case > > It looks so. > > > you can remove the '1 s.' from the comment below line 248 and maybe make it > > slightly clearer that the exception was thrown by this get(). > > Hmm, I'm missing this. > I need to timeout after 0.25 s (I forgot to update the comment below) because I > want to promptly detect crashes. > If I don't timeout and the process crashes, this code would keep waiting > undefinitely on the queue. Ah yeah, good point!
pliard@: I addressed the nits. bulach@: I moved to pylib and brought test coverage of the elf_symbolizer.py to 97% ;) I have not changed symbols.py yet. I'll do that in a separate CL and wait for that to be working before landing this (just to make eventual symbol.py reverts easier and not reverting this CL). Thanks both guys.
cool stuff, thanks! just nits and small suggestions.. I need to look into the test / mock files, will do so in a bit, just wanted to checkpoint in between mtgs.. https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:193: print >> self._proc.stdin, hex(addr) nit: probably best: self._proc.stdin.write('%s\n', hex(addr)) https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:228: # On timeout (1 s.) repeat the inner loop and check if either: a) the nit: isn't timeout 0.25 in 223? also, wouldn't it be better to wrap 223 with this exception and have a "continue" there? looks like the try..except block could be made smaller. https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:245: self._ProcessSymbolOutput(line1, line2) ditto, move this out of the try..except https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:254: self._proc.kill() nit: perhaps self._proc.wait() ? I think kill just signals, but doesn't wait... so the process may still be shutting down and using a lot of memory, and would prevent the next one from respawning. https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:343: return (a2l.queue_size, a2l.first_request_id) these two methods seem really trivial, wouldn't it be simpler to just inline them?! https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:346: def _StdoutReaderThread(process_pipe, queue): also, make this a @staticmethod in the class above rather than a top-level.. https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:361: except Exception: probably best to capture the more specific exceptions? https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer_unittest.py (right): https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer_unittest.py:29: def test_parallelism_1(self): I suppose we always use camelCaseTest? https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
lgtm, make sure phillipe is happy too! just some small comments... it'd be nice to change the third_party stack tool soon too! https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer_unittest.py (right): https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer_unittest.py:12: sys.path.insert(0, os.path.dirname(__file__)) nit: would this work, without the sys.path? from pylib import symbols from pylib.symbols import elf_symbolizer ... https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer_unittest.py:113: nit: add \n https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... File build/android/pylib/symbols/mock_addr2line/mock_addr2line (right): https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/mock_addr2line/mock_addr2line:29: sim_hang_period = int(os.environ.get('MOCK_A2L_HANG_EVERY', 0)) nit: keep it more consistent with the env, i.e., crash_every and hang_every.
https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:193: print >> self._proc.stdin, hex(addr) On 2014/02/20 17:00:13, bulach wrote: > nit: probably best: > > self._proc.stdin.write('%s\n', hex(addr)) Done. https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:228: # On timeout (1 s.) repeat the inner loop and check if either: a) the On 2014/02/20 17:00:13, bulach wrote: > nit: isn't timeout 0.25 in 223? > also, wouldn't it be better to wrap 223 with this exception and have a > "continue" there? looks like the try..except block could be made smaller. Ah right! https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:245: self._ProcessSymbolOutput(line1, line2) On 2014/02/20 17:00:13, bulach wrote: > ditto, move this out of the try..except Done. https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:254: self._proc.kill() On 2014/02/20 17:00:13, bulach wrote: > nit: perhaps self._proc.wait() ? I think kill just signals, but doesn't wait... > so the process may still be shutting down and using a lot of memory, and would > prevent the next one from respawning. Right. In general kill sends SIGKILL which should be almost immediate (and the new instance usually takes a while before eating memory), But I think a communicate (wait seems prone to deadlock if using PIPE, like in this case) shouldn't hurt. https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:343: return (a2l.queue_size, a2l.first_request_id) On 2014/02/20 17:00:13, bulach wrote: > these two methods seem really trivial, wouldn't it be simpler to just inline > them?! Actually I'll need GetUnknownSymbolPath outside for the outer level project-level symbolizer (when lib.so symbol is not found at all). Making it a static method. Ok for A2LSort... https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:346: def _StdoutReaderThread(process_pipe, queue): On 2014/02/20 17:00:13, bulach wrote: > also, make this a @staticmethod in the class above rather than a top-level.. Done. https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:361: except Exception: On 2014/02/20 17:00:13, bulach wrote: > probably best to capture the more specific exceptions? Hmm the problem is that I don't really know which kind of exceptions .readline() or .close() can happen when the process crashes (the point when it does that is pretty unpredictable) and I already check that the process is alway alive above. Any clues? https://codereview.chromium.org/167893009/diff/400001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/167893009/diff/400001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:328: def StdoutReaderThread(process_pipe, queue): Pylint is dumb and, if I use the _prefix, it complains about: Access to a protected member _StdoutReaderThread of a client class
still lgtm, clarification: https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:361: except Exception: On 2014/02/20 18:06:57, Primiano Tucci wrote: > On 2014/02/20 17:00:13, bulach wrote: > > probably best to capture the more specific exceptions? > > Hmm the problem is that I don't really know which kind of exceptions .readline() > or .close() can happen when the process crashes (the point when it does that is > pretty unpredictable) and I already check that the process is alway alive above. > Any clues? no idea, but I suppose IOError would be a good candidate? not really fine grained, but slightly clearer on what can be handled... :) the problem with a blanket Exception with pass is: python try: wdko21dko12d12d / 0 except Exception: print 'so what' that is, it swallows *everything*, from syntax errors all the way to runtime exceptions and everything in between... :)
https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:361: except Exception: On 2014/02/20 18:31:44, bulach wrote: > On 2014/02/20 18:06:57, Primiano Tucci wrote: > > On 2014/02/20 17:00:13, bulach wrote: > > > probably best to capture the more specific exceptions? > > > > Hmm the problem is that I don't really know which kind of exceptions > .readline() > > or .close() can happen when the process crashes (the point when it does that > is > > pretty unpredictable) and I already check that the process is alway alive > above. > > Any clues? > > no idea, but I suppose IOError would be a good candidate? not really fine > grained, but slightly clearer on what can be handled... :) the problem with a > blanket Exception with pass is: > > python > > try: > wdko21dko12d12d / 0 > except Exception: > print 'so what' > > > that is, it swallows *everything*, from syntax errors all the way to runtime > exceptions and everything in between... :) Ahh right, I didn't thought about that. Let's begin with (IOError and OSError) and see then what happens in the field :) https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer_unittest.py (right): https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer_unittest.py:12: sys.path.insert(0, os.path.dirname(__file__)) On 2014/02/20 17:35:58, bulach wrote: > nit: would this work, without the sys.path? > > from pylib import symbols > from pylib.symbols import elf_symbolizer > ... Hmm I think not (I'll give a try) because something has to add build/android to the PYTHONPATH. In my memory_inspector I dealt with that using run_tests as a launcher and running that from PRESUBMIT.py. But it doesn't look the ideal solution with every small sub-library (in this case that would mean moving the sys.path from this place to run_test which will only add another level of indirection). That why I was proposing a unified (i.e. non just buildbot related) chromium pylib. Maybe we can talk a bit about that next week that I'll be back in the base :) https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer_unittest.py:29: def test_parallelism_1(self): On 2014/02/20 17:00:13, bulach wrote: > I suppose we always use camelCaseTest? > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... Ah, right. I use that casing just by looking at python's unittes doc. Didn't realize that just matches test* https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer_unittest.py:113: On 2014/02/20 17:35:58, bulach wrote: > nit: add \n Done.
On 2014/02/20 18:59:13, Primiano Tucci wrote: > https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... > File build/android/pylib/symbols/elf_symbolizer.py (right): > > https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... > build/android/pylib/symbols/elf_symbolizer.py:361: except Exception: > On 2014/02/20 18:31:44, bulach wrote: > > On 2014/02/20 18:06:57, Primiano Tucci wrote: > > > On 2014/02/20 17:00:13, bulach wrote: > > > > probably best to capture the more specific exceptions? > > > > > > Hmm the problem is that I don't really know which kind of exceptions > > .readline() > > > or .close() can happen when the process crashes (the point when it does that > > is > > > pretty unpredictable) and I already check that the process is alway alive > > above. > > > Any clues? > > > > no idea, but I suppose IOError would be a good candidate? not really fine > > grained, but slightly clearer on what can be handled... :) the problem with a > > blanket Exception with pass is: > > > > python > > > > try: > > wdko21dko12d12d / 0 > > except Exception: > > print 'so what' > > > > > > that is, it swallows *everything*, from syntax errors all the way to runtime > > exceptions and everything in between... :) > > Ahh right, I didn't thought about that. > Let's begin with (IOError and OSError) and see then what happens in the field :) > > https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... > File build/android/pylib/symbols/elf_symbolizer_unittest.py (right): > > https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... > build/android/pylib/symbols/elf_symbolizer_unittest.py:12: sys.path.insert(0, > os.path.dirname(__file__)) > On 2014/02/20 17:35:58, bulach wrote: > > nit: would this work, without the sys.path? > > > > from pylib import symbols > > from pylib.symbols import elf_symbolizer > > ... > Hmm I think not (I'll give a try) because something has to add build/android to > the PYTHONPATH. > In my memory_inspector I dealt with that using run_tests as a launcher and > running that from PRESUBMIT.py. > But it doesn't look the ideal solution with every small sub-library (in this > case that would mean moving the sys.path from this place to run_test which will > only add another level of indirection). > That why I was proposing a unified (i.e. non just buildbot related) chromium > pylib. Maybe we can talk a bit about that next week that I'll be back in the > base :) > > https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... > build/android/pylib/symbols/elf_symbolizer_unittest.py:29: def > test_parallelism_1(self): > On 2014/02/20 17:00:13, bulach wrote: > > I suppose we always use camelCaseTest? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > Ah, right. I use that casing just by looking at python's unittes doc. Didn't > realize that just matches test* > > https://codereview.chromium.org/167893009/diff/310001/build/android/pylib/sym... > build/android/pylib/symbols/elf_symbolizer_unittest.py:113: > On 2014/02/20 17:35:58, bulach wrote: > > nit: add \n > > Done. lgtm, thanks!
Primiano, Can you please add BUG=339059? This is looking exciting. Can't wait to ditch the problematic java code in the binary size tool!
bulach@ I made some changes (Patchset 8) to support inlines, which was required to be fully compatible with the existing symbol.py. Can you PTAL? The matching change to symbols.py is here (not yet 100% ready but gives a good idea): http://crrev.com/164113003
lgtm, thanks a lot!
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/primiano@chromium.org/167893009/2
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/primiano@chromium.org/167893009/2
Message was sent while issue was closed.
Change committed as 252963 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
