Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
311443002:
elf_symbolizer: Use a process for max 4000 lookups and then restart (Closed)
|
Created:
6 years, 6 months ago by Daniel Bratell Modified:
6 years, 6 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionelf_symbolizer: Use a process for max 4000 lookups and then restart
addr2line processes keep growing as they are used so it's best to
restart them every now and then to avoid making its internal caches
so large that the computer runs out of RAM or the program becomes
incredibly slow.
I experimented with different counts. 10000 uses 50% more memory
than 4000 for a marginal performance increase. 1000 uses a bit
less memory but will be 10% slower. 3-4000 seems to be the sweat
spot.
BUG=379153
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275165
Patch Set 1 #
Total comments: 8
Patch Set 2 : Recycling: Addressed review comments. #
Total comments: 3
Patch Set 3 : addr2line count limit #Messages
Total messages: 14 (0 generated)
Seems reasonable, but please do a quick double check on that value of 1000. https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:190: self._max_requests_per_process = 1000 I think I left out a zero. I want to say this was more like 10,000. You should do some quick checks on your machine to see if you can notice an execution time difference with 1000 versus 10,000 here.
https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:190: self._max_requests_per_process = 1000 Two notes: You eventually want to make this an argument of ELFSymbolizer.__init__ (and propagate this here), not a hardcoded constant. I am also skeptical about the 1000. If you keep asking addr2line the same symbol 1000 times is not going to be any slow or eat any memory. Eventually, if you feel that 1000 is the magic number, you might want to pass 1000 in your binary size code. Essentially I am suggesting to add to line 71: ", recycle_addr2line_after=0". https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:191: self._queued_request_count = 0 You just need one "counter" variable, not two (see below). something like self._processed_symbols_count https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:206: self._queued_request_count += 1 This extra logic here is unnecessary. You can just kill addr2line whenever you feel like, basing on the number of replies. You don't need to count twice. Remove this hunk entirely. https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:232: self._queued_request_count -= self._received_answer_count This math seems rendudant. Here you just want to kill a2l if you exceeded the count, i.e.: if self._received_answer_count >= self._max_requests_per_process: self._processed_symbols_count = 0 self._RestartAddr2LineProcess() That's it, no? Also, I think you don't want do this check here. If the client is not fast enough, you might end up never hitting this path. What I'd do is adding a method RecycleIfNecessary() to this inner class, and call this method after line 119 (after a2l_to_purge.ProcessAllResolvedSymbolsInQueue())
Won't that cause addr2line to potentially do a lot of extra work that is discarded, because regardless when we kill it there will be data that has been processed by addr2line but hasn't yet been processed by elf_symbolizer? The reason for the double counter is to prevent us from wasting lookups since they are slow as they are.
On 2014/05/30 18:20:33, Daniel Bratell wrote: > Won't that cause addr2line to potentially do a lot of extra work that is > discarded, because regardless when we kill it there will be data that has been > processed by addr2line but hasn't yet been processed by elf_symbolizer? The > reason for the double counter is to prevent us from wasting lookups since they > are slow as they are. That's why I suggested to do that soon after .ProcessAllResolvedSymbolsInQueue(). After that point a2l has just been drained so you're not wasting anytyhing (Ok, very unlikely but possible, by the time you drain the queue in python a2l might have generated an extra symbol... but I don't think this is worth the extra complexity of all that boilerplate).
https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:190: self._max_requests_per_process = 1000 On 2014/05/30 15:42:29, Andrew Hayden wrote: > I think I left out a zero. I want to say this was more like 10,000. You should > do some quick checks on your machine to see if you can notice an execution time > difference with 1000 versus 10,000 here. I did some measurements at my computer. It becomes 10% faster by increasing to 3000 but after that the gains are small, if any. Max memory usage is then ~1.0 GB per process. With 10000 max memory usage is 1.5 GB per process. I will put 4000 as the limit, assuming most computers can handle a little bit more than the computer I use for this. https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:190: self._max_requests_per_process = 1000 On 2014/05/30 16:56:20, Primiano Tucci wrote: > Two notes: > You eventually want to make this an argument of ELFSymbolizer.__init__ (and > propagate this here), not a hardcoded constant. I am also skeptical about the > 1000. You were right, 1000 was too small. See measurements above. I've changed it to 4000. I don't think making it an argument is the best solution though. I see it as elf_symbolizer's job to shield the caller from addr2line peculiarities and I consider the inability of addr2line to run for a long time such a peculiarity. I will make it a constant though (and if someone is desperate they can reassign the value). https://codereview.chromium.org/311443002/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:232: self._queued_request_count -= self._received_answer_count On 2014/05/30 16:56:20, Primiano Tucci wrote: > This math seems rendudant. > Here you just want to kill a2l if you exceeded the count, i.e.: > > if self._received_answer_count >= self._max_requests_per_process: > self._processed_symbols_count = 0 > self._RestartAddr2LineProcess() > > That's it, no? > > Also, I think you don't want do this check here. > If the client is not fast enough, you might end up never hitting this path. > > What I'd do is adding a method RecycleIfNecessary() to this inner class, and > call this method after line 119 (after > a2l_to_purge.ProcessAllResolvedSymbolsInQueue()) > Seems it became a little bit faster by this change too. I guess they could idle a little with the previous version of the code.
LGTM. Thanks for addressing the concerns.
LGTM % 1 nit. Thanks https://codereview.chromium.org/311443002/diff/20001/build/android/pylib/symb... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/311443002/diff/20001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:197: self._processed_symbols_count = 0 Move this hunk in _RestartAddr2LineProcess, so you don't need to reset _processed_symbols_count = 0 twice (here and on line 272), but just on _Restart
https://codereview.chromium.org/311443002/diff/20001/build/android/pylib/symb... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/311443002/diff/20001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:197: self._processed_symbols_count = 0 On 2014/06/05 08:57:45, Primiano Tucci wrote: > Move this hunk in _RestartAddr2LineProcess, so you don't need to reset > _processed_symbols_count = 0 twice (here and on line 272), but just on _Restart It made pylint unhappy: W0201:348,6:ELFSymbolizer.Addr2Line._RestartAddr2LineProcess: Attribute '_processed_symbols_count' defined outside __init__ I agree that setting it inside _Restart is good though. Setting it only when restarted through the recycler was a bug.
Still LGTM https://codereview.chromium.org/311443002/diff/20001/build/android/pylib/symb... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/311443002/diff/20001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:197: self._processed_symbols_count = 0 On 2014/06/05 09:07:52, Daniel Bratell wrote: > On 2014/06/05 08:57:45, Primiano Tucci wrote: > > Move this hunk in _RestartAddr2LineProcess, so you don't need to reset > > _processed_symbols_count = 0 twice (here and on line 272), but just on > _Restart > > It made pylint unhappy: > W0201:348,6:ELFSymbolizer.Addr2Line._RestartAddr2LineProcess: Attribute > '_processed_symbols_count' defined outside __init__ > > I agree that setting it inside _Restart is good though. Setting it only when > restarted through the recycler was a bug. Oh right, you definitely need to define it in __init__ (but should be reset in restart). Good point
The CQ bit was checked by bratell@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/311443002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
Message was sent while issue was closed.
Change committed as 275165 |
