Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cache handler names #207

Merged
merged 2 commits into from
May 11, 2020
Merged

cache handler names #207

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
486fa1d
cache handler names
StrikerRUS May 8, 2020
647f3ec
compute maxsize for cache at runtime
StrikerRUS May 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 2 additions & 13 deletions m2cgen/interpreters/interpreter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import re

from m2cgen import ast
from m2cgen.interpreters.utils import CachedResult
from m2cgen.interpreters.utils import CachedResult, _get_handler_name


class BaseInterpreter:
Expand Down Expand Up @@ -55,21 +53,12 @@ def _reset_reused_expr_cache(self):
self._cached_expr_results = {}

def _select_handler(self, expr):
handler_name = self._handler_name(type(expr))
handler_name = _get_handler_name(type(expr))
if hasattr(self, handler_name):
return getattr(self, handler_name)
raise NotImplementedError(
"No handler found for '{}'".format(type(expr).__name__))

@staticmethod
def _handler_name(expr_tpe):
expr_name = BaseInterpreter._normalize_expr_name(expr_tpe.__name__)
return "interpret_" + expr_name

@staticmethod
def _normalize_expr_name(name):
return re.sub("(?!^)([A-Z]+)", r"_\1", name).lower()


class BaseToCodeInterpreter(BaseInterpreter):

Expand Down
13 changes: 13 additions & 0 deletions m2cgen/interpreters/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import re

from collections import namedtuple
from functools import lru_cache


CachedResult = namedtuple('CachedResult', ['var_name', 'expr_result'])
Expand All @@ -7,3 +10,13 @@
def get_file_content(path):
with open(path) as f:
return f.read()


@lru_cache(maxsize=16)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to put a larger number here to accommodate for future AST extension? I'd even suggest to make it unbounded if possible, since the total number of expressions in AST will always be pretty limited. Otherwise perhaps we can set it to something like 32.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that for the sake of efficiency it should be as small as possible. I even thought to set maxsize=8.
For future AST extension we can easily update the maxsize here while adding new expressions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my concern is that one should always keep this number on the back of their minds when adding new expression. Why do you say it should be as small as possible though? The cache is super cheap in this case, isn't it? It's if we just had a map with 16 pointer to string pairs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my concern is that one should always keep this number on the back of their minds when adding new expression.

Agree! It is not so convenient. Driving in a such direction we can simply replace two these functions with manual dict.

But according to the source code, lru_cache is based on linked list, not hashmap
https://github.com/python/cpython/blob/81a5fc38e81b424869f4710f48e9371dfa2d3b77/Lib/functools.py#L781

So I believe size matters here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, maybe I'm overoptimizing things. I've proposed new changes to this PR so that we won't have to care about keeping maxsize actual. Now it has enough capacity automatically. Please let me know what you think about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks 👍

def _get_handler_name(expr_tpe):
expr_name = _normalize_expr_name(expr_tpe.__name__)
return "interpret_" + expr_name


def _normalize_expr_name(name):
return re.sub("(?!^)([A-Z]+)", r"_\1", name).lower()