elf_symbolizer: Use a process for max 4000 lookups and then restart

build/android/pylib/symbols/elf_symbolizer.py

Index: build/android/pylib/symbols/elf_symbolizer.py
diff --git a/build/android/pylib/symbols/elf_symbolizer.py b/build/android/pylib/symbols/elf_symbolizer.py
index 90e99e5a2757770b30d8d3a89d0a34b589212780..25ae1a9f864fdf9c29242a582d6ad32195b08d68 100644
--- a/build/android/pylib/symbols/elf_symbolizer.py
+++ b/build/android/pylib/symbols/elf_symbolizer.py
@@ -184,6 +184,13 @@ class ELFSymbolizer(object):
       # separate field because turned out to be a perf hot-spot.
       self.queue_size = 0
 
+      # Keep track of requests count so that we can replace addr2line
+      # processes before they consume all the memory of the machine
+      # and become slow and evil.
+      self._max_requests_per_process = 1000

[Andrew Hayden (chromium.org), 2014/05/30 15:42:29]

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.

[Primiano Tucci (use gerrit), 2014/05/30 16:56:20]

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

[Daniel Bratell, 2014/06/04 15:31:16]

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.

[Daniel Bratell, 2014/06/04 15:31:16]

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

+      self._queued_request_count = 0

[Primiano Tucci (use gerrit), 2014/05/30 16:56:20]

You just need one "counter" variable, not two (see below).
something like self._processed_symbols_count

+      self._received_answer_count = 0
+
       # Objects required to handle the addr2line subprocess.
       self._proc = None  # Subprocess.Popen(...) instance.
       self._thread = None  # Threading.thread instance.
@@ -196,7 +203,10 @@ class ELFSymbolizer(object):
       req_idx = self._symbolizer.requests_counter
       self._request_queue.append((addr, callback_arg, req_idx))
       self.queue_size += 1
-      self._WriteToA2lStdin(addr)
+      self._queued_request_count += 1

[Primiano Tucci (use gerrit), 2014/05/30 16:56:20]

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.

+      if self._queued_request_count <= self._max_requests_per_process:
+        # Otherwise they will be written once we restart the process.
+        self._WriteToA2lStdin(addr)
 
     def WaitForIdle(self):
       """Waits until all the pending requests have been symbolized."""
@@ -215,6 +225,13 @@ class ELFSymbolizer(object):
 
         # The inner loop guards against a2l crashing (checking if it exited).
         while (datetime.datetime.now() - start_time < timeout):
+          # Restart the addr2line process if it has been running for a
+          # while. It keeps growing so otherwise it will make the
+          # whole computer unresponsive, including itself.
+          if self._received_answer_count >= self._max_requests_per_process:
+            self._queued_request_count -= self._received_answer_count

[Primiano Tucci (use gerrit), 2014/05/30 16:56:20]

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

[Daniel Bratell, 2014/06/04 15:31:16]

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.

+            self._received_answer_count = 0
+            self._RestartAddr2LineProcess()
           # poll() returns !None if the process exited. a2l should never exit.
           if self._proc.poll():
             logging.warning('addr2line crashed, respawning (lib: %s).' %
@@ -297,6 +314,7 @@ class ELFSymbolizer(object):
         if not innermost_sym_info:
           innermost_sym_info = sym_info
 
+      self._received_answer_count += 1
       self._symbolizer.callback(innermost_sym_info, callback_arg)
 
     def _RestartAddr2LineProcess(self):