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

build/android/pylib/symbols/elf_symbolizer.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import collections
 import datetime
 import logging
 import multiprocessing
 import os
 import posixpath
@@ skipping 166 matching lines @@
       self._lib_file_name = posixpath.basename(symbolizer.elf_file_path)
 
       # The request queue (i.e. addresses pushed to addr2line's stdin and not
       # yet retrieved on stdout)
       self._request_queue = collections.deque()
 
       # This is essentially len(self._request_queue). It has been optimized to a
       # 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.
       self._out_queue = None  # Queue.Queue instance (for buffering a2l stdout).
       self._RestartAddr2LineProcess()
 
     def EnqueueRequest(self, addr, callback_arg):
       """Pushes an address to addr2line's stdin (and keeps track of it)."""
       self._symbolizer.requests_counter += 1  # For global "age" of requests.
       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."""
       while self.queue_size > 0:
         self.WaitForNextSymbolInQueue()
 
     def WaitForNextSymbolInQueue(self):
       """Waits for the next pending request to be symbolized."""
       if not self.queue_size:
         return
 
       # This outer loop guards against a2l hanging (detecting stdout timeout).
       while True:
         start_time = datetime.datetime.now()
         timeout = datetime.timedelta(seconds=self._symbolizer.addr2line_timeout)
 
         # 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).' %
                             self._lib_file_name)
             self._RestartAddr2LineProcess()
             # TODO(primiano): the best thing to do in this case would be
             # shrinking the pool size as, very likely, addr2line is crashed
             # due to low memory (and the respawned one will die again soon).
 
           try:
@@ skipping 62 matching lines @@
               source_line = int(m.group(2))
         else:
           logging.warning('Got invalid symbol path from addr2line: %s' % line2)
 
         sym_info = ELFSymbolInfo(name, source_path, source_line)
         if prev_sym_info:
           prev_sym_info.inlined_by = sym_info
         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):
       if self._proc:
         self.Terminate()
 
       # The only reason of existence of this Queue (and the corresponding
       # Thread below) is the lack of a subprocess.stdout.poll_avail_lines().
       # Essentially this is a pipe able to extract a couple of lines atomically.
       self._out_queue = Queue.Queue()
@@ skipping 64 matching lines @@
     self.name = name
     self.source_path = source_path
     self.source_line = source_line
     # In the case of |inlines|=True, the |inlined_by| points to the outer
     # function inlining the current one (and so on, to form a chain).
     self.inlined_by = None
 
   def __str__(self):
     return '%s [%s:%d]' % (
         self.name or '??', self.source_path or '??', self.source_line or 0)