[CHORE] Commands optimizer (#4563)

A bit of performance detective work showed that the achievements code calls allCommands a lot, but allCommands actually parses the code, which is expensive. A few more tweaks could be made: * Fix the TODO "hacks" I made to get this done * Optimize even further by parsing once, and passing the parse tree to allCommands instead of parsing twice * If we expand the hypothesis testing, remove the allcommands call there too (it now parses twice)
hedyorg · Sep 27, 2023 · 0bf9276 · 0bf9276
1 parent 1a8df1b
commit 0bf9276
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 136 deletions.
diff --git a/.run/Python tests in test_level.run.xml b/.run/Python tests in test_level.run.xml
@@ -1,11 +1,12 @@
 <component name="ProjectRunConfigurationManager">
   <configuration default="false" name="Python tests in test_level" type="tests" factoryName="Autodetect" nameIsGenerated="true">
-    <module name="hedyorg" />
+    <module name="hedy" />
     <option name="INTERPRETER_OPTIONS" value="" />
     <option name="PARENT_ENVS" value="true" />
-    <option name="SDK_HOME" value="" />
+    <option name="SDK_HOME" value="$PROJECT_DIR$/venv/bin/python" />
+    <option name="SDK_NAME" value="Python 3.9 (hedy)" />
     <option name="WORKING_DIRECTORY" value="$PROJECT_DIR$" />
-    <option name="IS_MODULE_SDK" value="true" />
+    <option name="IS_MODULE_SDK" value="false" />
     <option name="ADD_CONTENT_ROOTS" value="true" />
     <option name="ADD_SOURCE_ROOTS" value="true" />
     <option name="_new_additionalArguments" value="&quot;&quot;" />

diff --git a/app.py b/app.py
@@ -553,13 +553,13 @@ def parse():
 
         with querylog.log_time('detect_sleep'):
             try:
-                response['has_sleep'] = 'sleep' in hedy.all_commands(code, level, lang)
+                response['has_sleep'] = 'sleep' in transpile_result.commands
             except BaseException:
                 pass
 
         try:
             if username and not body.get('tutorial') and ACHIEVEMENTS.verify_run_achievements(
-                    username, code, level, response):
+                    username, code, level, response, transpile_result.commands):
                 response['achievements'] = ACHIEVEMENTS.get_earned_achievements()
         except Exception as E:
             print(f"error determining achievements for {code} with {E}")

diff --git a/hedy.py b/hedy.py
@@ -258,20 +258,20 @@ def promote_types(types, rules):
     2: ['print', 'ask', 'is', 'turn', 'forward', 'color', 'sleep'],
     3: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from'],
     4: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'clear'],
-    5: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'clear'],
-    6: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'clear'],
-    7: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'repeat', 'times', 'clear'],
-    8: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'repeat', 'times', 'clear'],
-    9: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'repeat', 'times', 'clear'],
-    10: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'repeat', 'times', 'for', 'clear'],
-    11: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'for', 'range', 'repeat', 'clear'],
-    12: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'for', 'range', 'repeat', 'clear', 'define', 'call'],
-    13: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'for', 'range', 'repeat', 'and', 'or', 'clear', 'define', 'call'],
-    14: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'for', 'range', 'repeat', 'and', 'or', 'clear', 'define', 'call'],
-    15: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'for', 'range', 'repeat', 'and', 'or', 'while', 'clear', 'define', 'call'],
-    16: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'for', 'range', 'repeat', 'and', 'or', 'while', 'clear', 'define', 'call'],
-    17: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'pressed', 'button', 'for', 'range', 'repeat', 'and', 'or', 'while', 'elif', 'clear', 'define', 'call'],
-    18: ['is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'if', 'not in', 'else', 'for', 'pressed', 'button', 'range', 'repeat', 'and', 'or', 'while', 'elif', 'input', 'clear', 'define', 'call'],
+    5: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'clear'],
+    6: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'clear'],
+    7: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'repeat', 'times', 'clear'],
+    8: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'repeat', 'times', 'clear'],
+    9: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'repeat', 'times', 'clear'],
+    10: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'repeat', 'times', 'for', 'clear'],
+    11: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'for', 'range', 'repeat', 'clear'],
+    12: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'for', 'range', 'repeat', 'clear', 'define', 'call'],
+    13: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'for', 'range', 'repeat', 'and', 'or', 'clear', 'define', 'call'],
+    14: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'for', 'range', 'repeat', 'and', 'or', 'clear', 'define', 'call'],
+    15: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'for', 'range', 'repeat', 'and', 'or', 'while', 'clear', 'define', 'call'],
+    16: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'for', 'range', 'repeat', 'and', 'or', 'while', 'clear', 'define', 'call'],
+    17: ['ask', 'is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'not in', 'if', 'else', 'ifpressed', 'assign_button', 'for', 'range', 'repeat', 'and', 'or', 'while', 'elif', 'clear', 'define', 'call'],
+    18: ['is', 'print', 'forward', 'turn', 'color', 'sleep', 'at', 'random', 'add', 'to', 'remove', 'from', 'in', 'if', 'not in', 'else', 'for', 'ifpressed', 'assign_button', 'range', 'repeat', 'and', 'or', 'while', 'elif', 'input', 'clear', 'define', 'call'],
 }
 
 command_turn_literals = ['right', 'left']
@@ -357,6 +357,10 @@ def get_list_keywords(commands, to_lang):
         with open(to_yaml_filesname_with_path, 'r', encoding='utf-8') as stream:
             to_yaml_dict = yaml.safe_load(stream)
         for command in commands:
+            if command == 'ifpressed':  # TODO: this is a bit of a hack
+                command = 'pressed'    # since in the yamls they are called pressed
+            if command == 'assign_button':  # but in the grammar 'ifpressed'
+                command = 'button'         # should be changed in the yaml eventually!
             try:
                 translation_commands.append(to_yaml_dict[command])
             except Exception:
@@ -978,40 +982,6 @@ def text(self, meta, args):
         return all(args), ''.join([c for c in args]), meta
 
 
-class UsesTurtle(Transformer):
-    def __default__(self, args, children, meta):
-        if len(children) == 0:  # no children? you are a leaf that is not Turn or Forward, so you are no Turtle command
-            return False
-        else:
-            return any(isinstance(c, bool) and c is True for c in children)
-
-    # returns true if Forward or Turn are in the tree, false otherwise
-    def forward(self, args):
-        return True
-
-    def color(self, args):
-        return True
-
-    def turn(self, args):
-        return True
-
-    # somehow tokens are not picked up by the default rule so they need their own rule
-    def INT(self, args):
-        return False
-
-    def NAME(self, args):
-        return False
-
-    def NUMBER(self, args):
-        return False
-
-    def POSITIVE_NUMBER(self, args):
-        return False
-
-    def NEGATIVE_NUMBER(self, args):
-        return False
-
-
 class UsesPyGame(Transformer):
     command_prefix = (f"""\
 pygame_end = False
@@ -1023,21 +993,6 @@ class UsesPyGame(Transformer):
     pygame.quit()
     break""")
 
-    def __default__(self, args, children, meta):
-        if len(children) == 0:  # no children? you are a leaf that is not Pressed, so you are no PyGame command
-            return False
-        else:
-            return any(isinstance(c, bool) and c is True for c in children)
-
-    def ifpressed(self, args):
-        return True
-
-    def ifpressed_else(self, args):
-        return True
-
-    def assign_button(self, args):
-        return True
-
 
 class AllCommands(Transformer):
     def __init__(self, level):
@@ -1077,10 +1032,12 @@ def __default__(self, args, children, meta):
         # if we are matching a rule that is a command
         production_rule_name = self.translate_keyword(args)
         leaves = flatten_list_of_lists_to_list(children)
-        # for the achievements we want to be able to also detct which operators were used by a kid
+        # for the achievements we want to be able to also detect which operators were used by a kid
         operators = ['addition', 'subtraction', 'multiplication', 'division']
 
-        if production_rule_name in commands_per_level[self.level] or production_rule_name in operators:
+        if production_rule_name in commands_per_level[self.level] or production_rule_name in operators or production_rule_name == 'ifpressed_else':
+            # ifpressed_else is not in the yamls, upsetting lookup code to get an alternative later
+            # lookup should be fixed instead, making a special case for now
             if production_rule_name == 'else':  # use of else also has an if
                 return ['if', 'else'] + leaves
             return [production_rule_name] + leaves
@@ -1120,48 +1077,6 @@ def all_commands(input_string, level, lang='en'):
     return AllCommands(level).transform(program_root)
 
 
-class AllPrintArguments(Transformer):
-    def __init__(self, level):
-        self.level = level
-
-    def __default__(self, args, children, meta):
-        leaves = flatten_list_of_lists_to_list(children)
-
-        if args == 'print':
-            return children
-        else:
-            return leaves  # 'pop up' the children
-
-    def program(self, args):
-        return flatten_list_of_lists_to_list(args)
-
-    # somehow tokens are not picked up by the default rule so they need their own rule
-    def INT(self, args):
-        return []
-
-    def NAME(self, args):
-        return []
-
-    def NUMBER(self, args):
-        return []
-
-    def POSITIVE_NUMBER(self, args):
-        return []
-
-    def NEGATIVE_NUMBER(self, args):
-        return []
-
-    def text(self, args):
-        return ''.join(args)
-
-
-def all_print_arguments(input_string, level, lang='en'):
-    input_string = process_input_string(input_string, level, lang)
-    program_root = parse_input(input_string, level, lang)
-
-    return AllPrintArguments(level).transform(program_root)
-
-
 @v_args(meta=True)
 class IsValid(Filter):
     # all rules are valid except for the "Invalid" production rule
@@ -2710,7 +2625,7 @@ def get_parser(level, lang="en", keep_all_tokens=False):
     return ret
 
 
-ParseResult = namedtuple('ParseResult', ['code', 'source_map', 'has_turtle', 'has_pygame'])
+ParseResult = namedtuple('ParseResult', ['code', 'source_map', 'has_turtle', 'has_pygame', 'commands'])
 
 
 def transpile_inner_with_skipping_faulty(input_string, level, lang="en"):
@@ -2772,6 +2687,7 @@ def transpile(input_string, level, lang="en", skip_faulty=True):
     try:
         source_map.set_skip_faulty(False)
         transpile_result = transpile_inner(input_string, level, lang, populate_source_map=True)
+
     except Exception as original_error:
         if getenv('ENABLE_SKIP_FAULTY', False) and skip_faulty:
             if isinstance(original_error, source_map.exceptions_not_to_skip):
@@ -3271,16 +3187,16 @@ def transpile_inner(input_string, level, lang="en", populate_source_map=False):
         convertToPython = TRANSPILER_LOOKUP[level]
         python = convertToPython(lookup_table, numerals_language).transform(abstract_syntax_tree)
 
-        uses_turtle = UsesTurtle()
-        has_turtle = uses_turtle.transform(abstract_syntax_tree)
+        # note to self, we still parse twice, we can also pass all_commands the ast!
+        commands = all_commands(input_string, level, lang)
 
-        uses_pygame = UsesPyGame()
-        has_pygame = uses_pygame.transform(abstract_syntax_tree)
+        has_turtle = "forward" in commands or "turn" in commands or "color" in commands
+        has_pygame = "ifpressed" in commands or "ifpressed_else" in commands or "assign_button" in commands
 
         if populate_source_map:
             source_map.set_python_output(python)
 
-        return ParseResult(python, source_map, has_turtle, has_pygame)
+        return ParseResult(python, source_map, has_turtle, has_pygame, commands)
     except VisitError as E:
         if isinstance(E, VisitError):
             # Exceptions raised inside visitors are wrapped inside VisitError. Unwrap it if it is a

diff --git a/tests/Tester.py b/tests/Tester.py
@@ -268,7 +268,7 @@ def single_level_tester(
                                 in_english, from_lang="en", to_lang=lang, level=self.level)
                             self.assert_translated_code_equal(code, back_in_org)
 
-                    all_commands = hedy.all_commands(code, level, lang)
+                    all_commands = result.commands
                     if expected_commands is not None:
                         self.assertEqual(expected_commands, all_commands)
                     # <- use this to run tests locally with unittest

diff --git a/tests/test_level/test_level_01.py b/tests/test_level/test_level_01.py
@@ -38,8 +38,8 @@ def test_print(self):
             expected=expected,
             output=output,
             expected_commands=expected_commands
+
         )
-        self.assertEqual([output], hedy.all_print_arguments(code, self.level))
 
     def test_print_no_space(self):
         code = "printHallo welkom bij Hedy!"
@@ -53,7 +53,6 @@ def test_print_no_space(self):
             output=output,
             expected_commands=expected_commands
         )
-        self.assertEqual([output], hedy.all_print_arguments(code, self.level))
 
     def test_print_line_with_spaces_works(self):
         code = "print hallo\n      \nprint hallo"
@@ -83,7 +82,6 @@ def test_print_multiple_lines(self):
         Mooi hoor""")
 
         self.single_level_tester(code=code, expected=expected, output=output)
-        self.assertEqual(['Hallo welkom bij Hedy', 'Mooi hoor'], hedy.all_print_arguments(code, self.level))
 
     def test_print_single_quoted_text(self):
         code = "print 'Welcome to OceanView!'"
@@ -519,8 +517,6 @@ def test_print_comment(self):
             expected_commands=[Command.print]
         )
 
-        self.assertEqual(['Hallo welkom bij Hedy! '], hedy.all_print_arguments(code, self.level))
-
     #
     # combined commands tests
     #
@@ -793,5 +789,9 @@ def test_template_combination(self, code_tuples, d):
             expected_commands = [Command.ask, Command.ask, Command.echo, Command.echo, Command.forward, Command.forward,
                                  Command.print, Command.print, Command.print, Command.turn, Command.turn]
 
+            # TODO, FH sept 2023: all_commands parses and thus is expensive
+            # we should get the commands list back from the parser instead (parseresult.commands)
+            # since we don't use many single_level_tester features
+            # we can transpile and check the python "manually"
             all_commands = sorted(hedy.all_commands(code, self.level, 'en'))
             self.assertEqual(expected_commands, all_commands)