[Dexter] Improve Dexter's performance by evaluating expressions only when needed

[StephenTozer]

Summary

Currently, Dexter's model for fetching watch values is to build a list of expressions to watch before running the debugger, then evaluating all of them at each breakpoint, then finally looking up the values of these expressions at each line they were expected on. When using dexter on a large project while watching many different expressions, this is very slow, as Dexter will make a massive number of calls made to the debugger's API, the vast majority of which are not being used for anything. This patch fixes this issue by having Dexter only evaluate expressions at a breakpoint when it will be used by a Dexter command.

[chrisjackson]

A couple of nits, really nice patch though! Its really going to improve things.

cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectProgramState.py

14–1

I'm keen on ternary ifs myself, but coupled with the construction of a StepExpectInfo in a from_iterable I feel this block could be a little hard to read. But I concede its just personal preference.

cross-project-tests/debuginfo-tests/dexter/dex/debugger/DebuggerBase.py

18

Possibly awkward to read combination of and, not, or. Is there a nicer way to expess this? Just a nit really.

[jmorse]

LGTM in principle, although I agree with Chris, the get_watches block is a bit too pythonic, and contains a generator within a generator. It'd be best to split that into statements, IMO.

It's not ideal that the debugger wrappers have to be adjusted to benefit from these improvements; I guess that's a future refactoring project though.

cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectWatchBase.py

3

I have mixed feelings about this; IMO we want to steer away from relying on the source files being accessible by Dexter while running. But on the other hand, the previous implementation on this function relied on it for correctness, all this assert is doing is drawing attention to the fact that if the file isn't there, you won't get the results you wanted.

Overall I would call this an improvement, as it accelerates our likelyhood of fixing this.

cross-project-tests/debuginfo-tests/dexter/dex/debugger/DebuggerBase.py

16–17

Adding a type annotation, docu-comment or otherwise to indicate the type of watch_info would be good. It'll be hard for returning readers otherwise.

[StephenTozer]

Add comments and type annotations, split complex statements out across multiple lines.

[StephenTozer]

Reupload previous diff with -U999999 flag.

[jmorse]

LGTM with one inline question -- if that's an issue, fine to land with the obvious fix.

cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectProgramState.py

27

Is this going to lead to multiple StepExpectInfo objects referring to the same range object? And if so, doesn't that mean consuming the range in one frame will consume it in all the frames?

(If so, easily fixed by converting to a list).

[StephenTozer]

cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectProgramState.py

27

I don't think so - the frame index is in the StepExpectInfo tuple, so unless the same value is being watched twice in the same stack frame there shouldn't be duplicates (and if there are then we should merge them).

[jmorse]

cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectProgramState.py

27

Possibly I've misread, but if there are multiple watches (line 66) in a frame, then multiple StepExpectInfo's will be constructed, each referring to the range object constructed on line 63, no?

[StephenTozer]

cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectProgramState.py

27

Ah, I understand now - this isn't an issue, because python range objects are not consumed on use; they're an immutable object, and they must be in order to be used as part of a set element. For the same reason, we can't use a list here (because a list is a mutable object). This means that it should be perfectly safe to pass around the same object into every set.

cross-project-tests/debuginfo-tests/dexter/dex/command/CommandBase.py

 # DExTer : Debugging Experience Tester
 # ~~~~~~   ~         ~~         ~   ~~
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 """Base class for all DExTer commands, where a command is a specific Python
 function that can be embedded into a comment in the source code under test
 which will then be executed by DExTer during debugging.
 """
 
 import abc
+from collections import namedtuple
 from typing import List
 
+StepExpectInfo = namedtuple('StepExpectInfo', 'expression, path, frame_idx, line_range')
+
 class CommandBase(object, metaclass=abc.ABCMeta):
     def __init__(self):
         self.path = None
         self.lineno = None
         self.raw_text = ''
 
     def get_label_args(self):
         return list()

cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectProgramState.py

 # DExTer : Debugging Experience Tester
 # ~~~~~~   ~         ~~         ~   ~~
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 """Command for specifying a partial or complete state for the program to enter
 during execution.
 """
 
 from itertools import chain
 
-from dex.command.CommandBase import CommandBase
+from dex.command.CommandBase import CommandBase, StepExpectInfo
 from dex.dextIR import ProgramState, SourceLocation, StackFrame, DextIR
 
 def frame_from_dict(source: dict) -> StackFrame:
     if 'location' in source:
         assert isinstance(source['location'], dict)
         source['location'] = SourceLocation(**source['location'])
     return StackFrame(**source)
 
 def state_from_dict(source: dict) -> ProgramState:
     if 'frames' in source:
         assert isinstance(source['frames'], list)
         source['frames'] = list(map(frame_from_dict, source['frames']))
     return ProgramState(**source)
 
 class DexExpectProgramState(CommandBase):
     """Expect to see a given program `state` a certain numer of `times`.
 
     DexExpectProgramState(state [,**times])
 
     See Commands.md for more info.
     """
 
def __init__(self, *args, **kwargs):
 
         super(DexExpectProgramState, self).__init__()
 
     @staticmethod
     def get_name():
         return __class__.__name__
 
     def get_watches(self):
-        frame_expects = chain.from_iterable(frame.watches
-            for frame in self.expected_program_state.frames)
-        return set(frame_expects)
+        frame_expects = set()
+        for idx, frame in enumerate(self.expected_program_state.frames):
+            path = (frame.location.path if
+                    frame.location and frame.location.path else self.path)
+            line_range = (
+                range(frame.location.lineno, frame.location.lineno + 1)
+                if frame.location and frame.location.lineno else None)
+            for watch in frame.watches:
+                frame_expects.add(
+                    StepExpectInfo(
+                        expression=watch,
+                        path=path,
+                        frame_idx=idx,
+                        line_range=line_range
+                    )
+                )
+        return frame_expects
 
     def eval(self, step_collection: DextIR) -> bool:
         for step in step_collection.steps:
             if self.expected_program_state.match(step.program_state):
                 self.encounters.append(step.step_index)
 
         return self.times < 0 < len(self.encounters) or len(self.encounters) == self.times

cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectWatchBase.py

 # DExTer : Debugging Experience Tester
 # ~~~~~~   ~         ~~         ~   ~~
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 """DexExpectWatch base class, holds logic for how to build and process expected
  watch commands.
 """
 
 import abc
 import difflib
 import os
+from collections import namedtuple
 
-from dex.command.CommandBase import CommandBase
+from dex.command.CommandBase import CommandBase, StepExpectInfo
 from dex.command.StepValueInfo import StepValueInfo
 
 
+
 class DexExpectWatchBase(CommandBase):
     def __init__(self, *args, **kwargs):
         if len(args) < 2:
             raise TypeError('expected at least two args')
 
         self.expression = args[0]
         self.values = [str(arg) for arg in args[1:]]
         try:
def __init__(self, *args, **kwargs):
         # List of StepValueInfos for any watch that is encountered that has an
         # unexpected value.
         self.unexpected_watches = []
 
         super(DexExpectWatchBase, self).__init__()
 
 
     def get_watches(self):
-        return [self.expression]
+        return [StepExpectInfo(self.expression, self.path, 0, range(self._from_line, self._to_line + 1))]
 
     @property
     def line_range(self):
         return list(range(self._from_line, self._to_line + 1))
 
     @property
     def missing_values(self):
         return sorted(list(self._missing_values))
def _check_watch_order(self, actual_watches, expected_values):
                 differences.append(actual_watches[index])
                 index += 1
             else:
                 assert False, 'unexpected diff:{}'.format(vd)
 
         return differences
 
     def eval(self, step_collection):
+        assert os.path.exists(self.path)
         for step in step_collection.steps:
             loc = step.current_location
 
             if (loc.path and os.path.exists(loc.path) and
-                os.path.exists(self.path) and
                 os.path.samefile(loc.path, self.path) and
                 loc.lineno in self.line_range):
                 try:
                     watch = step.program_state.frames[0].watches[self.expression]
                 except KeyError:
                     pass
                 else:
                     expected_field = self._get_expected_field(watch)

cross-project-tests/debuginfo-tests/dexter/dex/debugger/DebuggerBase.py

 # DExTer : Debugging Experience Tester
 # ~~~~~~   ~         ~~         ~   ~~
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 """Base class for all debugger interface implementations."""
 
 import abc
 import os
 import sys
 import traceback
 import unittest
 
 from types import SimpleNamespace
+from dex.command.CommandBase import StepExpectInfo
 from dex.dextIR import DebuggerIR, FrameIR, LocIR, StepIR, ValueIR
 from dex.utils.Exceptions import DebuggerException
 from dex.utils.Exceptions import NotYetLoadedDebuggerException
 from dex.utils.ReturnCode import ReturnCode
 
+def watch_is_active(watch_info: StepExpectInfo, path, frame_idx, line_no):
+    _, watch_path, watch_frame_idx, watch_line_range = watch_info
+    # If this watch should only be active for a specific file...
+    if watch_path and os.path.isfile(watch_path):
+        # If the current path does not match the expected file, this watch is
+        # not active.
+        if not (path and os.path.isfile(path) and
+                os.path.samefile(path, watch_path)):
+            return False
+    if watch_frame_idx != frame_idx:
+        return False
+    if watch_line_range and line_no not in list(watch_line_range):
+        return False
+    return True
 
 class DebuggerBase(object, metaclass=abc.ABCMeta):
     def __init__(self, context):
         self.context = context
         # Note: We can't already read values from options
         # as DebuggerBase is created before we initialize options
         # to read potential_debuggers.
         self.options = self.context.options

cross-project-tests/debuginfo-tests/dexter/dex/debugger/dbgeng/dbgeng.py

 # DExTer : Debugging Experience Tester
 # ~~~~~~   ~         ~~         ~   ~~
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 import sys
 import os
 import platform
 
-from dex.debugger.DebuggerBase import DebuggerBase
+from dex.debugger.DebuggerBase import DebuggerBase, watch_is_active
 from dex.dextIR import FrameIR, LocIR, StepIR, StopReason, ValueIR
 from dex.dextIR import ProgramState, StackFrame, SourceLocation
 from dex.utils.Exceptions import DebuggerException, LoadDebuggerException
 from dex.utils.ReturnCode import ReturnCode
 
 if platform.system() == "Windows":
   # Don't load on linux; _load_interface will croak before any names are used.
   from . import setup
def _get_step_info(self, watches, step_index):
 
           state_frame = StackFrame(function=new_frame.function,
                                    is_inlined=new_frame.is_inlined,
                                    location=SourceLocation(path=x.source_file,
                                                            lineno=x.line_no,
                                                            column=0),
                                    watches={})
           for expr in map(
-              lambda watch, idx=i: self.evaluate_expression(watch, idx),
-              watches):
+              # Filter out watches that are not active in the current frame,
+              # and then evaluate all the active watches.
+              lambda watch_info, idx=i:
+                self.evaluate_expression(watch_info.expression, idx),
+              filter(
+                  lambda watch_info, idx=i, line_no=loc.lineno, path=loc.path:
+                    watch_is_active(watch_info, path, idx, line_no),
+                  watches)):
               state_frame.watches[expr.expression] = expr
           state_frames.append(state_frame)
 
         return StepIR(
             step_index=step_index, frames=dex_frames,
             stop_reason=StopReason.STEP,
             program_state=ProgramState(state_frames))
 

cross-project-tests/debuginfo-tests/dexter/dex/debugger/lldb/LLDB.py

 # DExTer : Debugging Experience Tester
 # ~~~~~~   ~         ~~         ~   ~~
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 """Interface for communicating with the LLDB debugger via its python interface.
 """
 
 import imp
 import os
 from subprocess import CalledProcessError, check_output, STDOUT
 import sys
 
-from dex.debugger.DebuggerBase import DebuggerBase
+from dex.debugger.DebuggerBase import DebuggerBase, watch_is_active
 from dex.dextIR import FrameIR, LocIR, StepIR, StopReason, ValueIR
 from dex.dextIR import StackFrame, SourceLocation, ProgramState
 from dex.utils.Exceptions import DebuggerException, LoadDebuggerException
 from dex.utils.ReturnCode import ReturnCode
 
 
 class LLDB(DebuggerBase):
     def __init__(self, context, *args):
def _get_step_info(self, watches, step_index):
             function = self._sanitize_function_name(sb_frame.GetFunctionName())
 
             loc_dict = {
                 'path': path,
                 'lineno': sb_line.GetLine(),
                 'column': sb_line.GetColumn()
             }
             loc = LocIR(**loc_dict)
+            valid_loc_for_watch = loc.path and os.path.exists(loc.path)
 
             frame = FrameIR(
                 function=function, is_inlined=sb_frame.IsInlined(), loc=loc)
 
             if any(
                     name in (frame.function or '')  # pylint: disable=no-member
                     for name in self.frames_below_main):
                 break
 
             frames.append(frame)
 
             state_frame = StackFrame(function=frame.function,
                                      is_inlined=frame.is_inlined,
                                      location=SourceLocation(**loc_dict),
                                      watches={})
+            if valid_loc_for_watch:
                 for expr in map(
-                lambda watch, idx=i: self.evaluate_expression(watch, idx),
-                watches):
+                    # Filter out watches that are not active in the current frame,
+                    # and then evaluate all the active watches.
+                    lambda watch_info, idx=i:
+                        self.evaluate_expression(watch_info.expression, idx),
+                    filter(
+                        lambda watch_info, idx=i, line_no=loc.lineno, loc_path=loc.path:
+                            watch_is_active(watch_info, loc_path, idx, line_no),
+                        watches)):
                     state_frame.watches[expr.expression] = expr
                 state_frames.append(state_frame)
 
         if len(frames) == 1 and frames[0].function is None:
             frames = []
             state_frames = []
 
         reason = self._translate_stop_reason(self._thread.GetStopReason())
 
         return StepIR(

cross-project-tests/debuginfo-tests/dexter/dex/debugger/visualstudio/VisualStudio.py

 # DExTer : Debugging Experience Tester
 # ~~~~~~   ~         ~~         ~   ~~
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 """Interface for communicating with the Visual Studio debugger via DTE."""
 
 import abc
 import imp
 import os
 import sys
 from pathlib import PurePath
-from collections import namedtuple
-from collections import defaultdict
+from collections import defaultdict, namedtuple
 
-from dex.debugger.DebuggerBase import DebuggerBase
+from dex.command.CommandBase import StepExpectInfo
+from dex.debugger.DebuggerBase import DebuggerBase, watch_is_active
 from dex.dextIR import FrameIR, LocIR, StepIR, StopReason, ValueIR
 from dex.dextIR import StackFrame, SourceLocation, ProgramState
 from dex.utils.Exceptions import Error, LoadDebuggerException
 from dex.utils.ReturnCode import ReturnCode
 
 
 def _load_com_module():
     try:
class VisualStudio(DebuggerBase, metaclass=abc.ABCMeta):  # pylint: disable=abstract-method
     def _get_step_info(self, watches, step_index):
         thread = self._debugger.CurrentThread
         stackframes = thread.StackFrames
 
         frames = []
         state_frames = []
 
 
+        loc = LocIR(**self._location)
+        valid_loc_for_watch = loc.path and os.path.exists(loc.path)
+
         for idx, sf in enumerate(stackframes):
             frame = FrameIR(
                 function=self._sanitize_function_name(sf.FunctionName),
                 is_inlined=sf.FunctionName.startswith('[Inline Frame]'),
                 loc=LocIR(path=None, lineno=None, column=None))
 
             fname = frame.function or ''  # pylint: disable=no-member
             if any(name in fname for name in self.frames_below_main):
                 break
 
-
             state_frame = StackFrame(function=frame.function,
                                      is_inlined=frame.is_inlined,
                                      watches={})
 
-            for watch in watches:
-                state_frame.watches[watch] = self.evaluate_expression(
-                    watch, idx)
+            if valid_loc_for_watch and idx == 0:
+                for watch_info in watches:
+                    if watch_is_active(watch_info, loc.path, idx, loc.lineno):
+                        watch_expr = watch_info.expression
+                        state_frame.watches[watch_expr] = self.evaluate_expression(watch_expr, idx)
 
 
             state_frames.append(state_frame)
             frames.append(frame)
 
-        loc = LocIR(**self._location)
         if frames:
             frames[0].loc = loc
             state_frames[0].location = SourceLocation(**self._location)
 
         reason = StopReason.BREAKPOINT
         if loc.path is None:  # pylint: disable=no-member
             reason = StopReason.STEP
 
class VisualStudio(DebuggerBase, metaclass=abc.ABCMeta):  # pylint: disable=abstract-method
     @property
     def frames_below_main(self):
         return [
             '[Inline Frame] invoke_main', '__scrt_common_main_seh',
             '__tmainCRTStartup', 'mainCRTStartup'
         ]
 
     def evaluate_expression(self, expression, frame_idx=0) -> ValueIR:
+        if frame_idx != 0:
             self.set_current_stack_frame(frame_idx)
         result = self._debugger.GetExpression(expression)
+        if frame_idx != 0:
             self.set_current_stack_frame(0)
         value = result.Value
 
         is_optimized_away = any(s in value for s in [
             'Variable is optimized away and not available',
             'Value is not available, possibly due to optimization',
         ])
 
         is_irretrievable = any(s in value for s in [