Add caching to bottlenecks in the message store #5605
Conversation
Some functions can't be cached without impacting the corectness with the current design.
Pull Request Test Coverage Report for Build 1633545280
💛 - Coveralls |
Co-authored-by: Daniël van Noord <[email protected]>
|
@Pierre-Sassoulas Should we add a limit to the cache size? |
Sorry I thought I added what I was thinking about that to the description but evidently I did not. Regarding msgid / symbol correspondence ( Previously, I was also caching |
|
I think we should add either such a comment or a limit to it, to avoid future confusion. |
Co-authored-by: Daniël van Noord <[email protected]>
| msgid: Optional[str] | ||
| symbol: Optional[str] | ||
| if msgid_or_symbol[1:].isdigit(): | ||
| # Only msgid can have a digit as second letter | ||
| msgid = msgid_or_symbol.upper() |
There was a problem hiding this comment.
| msgid = msgid_or_symbol.upper() | |
| msgid: Optional[str] = msgid_or_symbol.upper() |
Don't know why I didn't think of this sooner, but this should fix it (or this change but then on L118. We don't need an extra line, we just need to add typing to the call that casts either to str instead of Optional[str].
There was a problem hiding this comment.
Well I did that at first but then self.__msgid_to_symbol.get(msgid) expects a string and not a null-able string.
There was a problem hiding this comment.
Why did we remove the msgid = None line?
def get_active_msgids(self, msgid_or_symbol: str) -> List[str]:
"""Return msgids but the input can be a symbol."""
# Only msgid can have a digit as second letter
msgid = None
if msgid_or_symbol[1:].isdigit():
msgid = msgid_or_symbol.upper()
symbol = self.__msgid_to_symbol.get(msgid)
else:
msgid = self.__symbol_to_msgid.get(msgid_or_symbol)
symbol = msgid_or_symbol
if msgid is None or symbol is None or not msgid or not symbol:
error_msg = f"No such message id or symbol '{msgid_or_symbol}'."
raise UnknownMessageError(error_msg)
return self.__old_names.get(msgid, [msgid])Gives not errors for me with mypy --strict. mypy infers that msgid is Optional[str] I think.
There was a problem hiding this comment.
I suppose the type hint is only for type checker when the assignment is really executed at run time and impact performance.
Type of Changes
Description
This add caching to bottlenecks in the MessageStore. Next step would be to generate the code of an immutable data structure from the checkers instead of creating and checking it at runtime and then using this structure directly, but I think caching is fine for now. (Especially as we can't do everything this way: For plugin there are still unexpected message id and symbol.)
get_active_msgidswas calling .lower() / is_digit on every message id we gave it every time caching it save a lot of call.There's some function we can't cache because of the way PyLinter is designed.
Tested by linting django (03cadb912c78b769d6bf4a943a2a35fc1d952960) with
python3 -m cProfile -m pylint .pylint_primer_tests/django|grep messageBefore:
After
Follow up to #4814