Memory Optimizations#5222
Conversation
- removed dead fields and redundant vars - removed unused import
cobaltt7
left a comment
cobaltt7
left a comment
There was a problem hiding this comment.
Thanks! Could you add a high-level changelog entry for one or two of the biggest changes?
| type: int # int: token number (< 256) or symbol number (>= 256) | ||
| parent: Optional["Node"] = None # Parent node pointer, or None | ||
| children: list[NL] # List of subnodes | ||
| was_changed: bool = False |
There was a problem hiding this comment.
Could you migrate these comments into __init__?
| value: str, | ||
| context: Context | None = None, | ||
| prefix: str | None = None, | ||
| fixers_applied: list[Any] = [], |
There was a problem hiding this comment.
Is this argument now unused?
There was a problem hiding this comment.
Yes, fixer_applied is now unused in both Node and Leaf. But since Node had unused 'context' param, so I kept this param as is.
|
Did you compare this under the mypyc build? slots shouldn't make that big of a difference after being compiled |
JelleZijlstra
left a comment
JelleZijlstra
left a comment
There was a problem hiding this comment.
This makes a lot of stylistic changes, many of which are unlikely to affect memory usage.
You claim this reduced memory usage significantly. How did you measure this? Did you compile Black with mypyc? And which specific changes can be proven to help?
|
|
||
| priority = priority or self.max_delimiter_priority() | ||
| return sum(1 for p in self.delimiters.values() if p == priority) | ||
| return sum(p == priority for p in self.delimiters.values()) |
There was a problem hiding this comment.
Why would this reduce memory usage? Python won't allocate a new 1 object.
There was a problem hiding this comment.
It is mainly a readability/simplicity change, not a memory optimization.
| comments_iter = itertools.chain.from_iterable(self.comments.values()) | ||
| comments = [str(comment) for comment in comments_iter] | ||
| res += "".join(comments) | ||
| first, *rest = self.leaves |
There was a problem hiding this comment.
The new code overall seems nicer here but note this materializes rest into a new list and therefore might use more memory.
There was a problem hiding this comment.
Thanks to point it out.
Since self.leaves is already a list, hence now using it directly instead of creating iter overhead.
| group of custom splits. | ||
| """ | ||
| return (id(string), string) | ||
| return id(string), string |
There was a problem hiding this comment.
This definitely doesn't affect memory usage.
There was a problem hiding this comment.
Yes, its redundant parenthesis hence removed.
| for start, end in iter_fexpr_spans(fstring): | ||
| parts.append(fstring[previous_index:start]) | ||
| parts.append(fstring[start:end].replace(old_quote, new_quote)) | ||
| parts.extend(( |
There was a problem hiding this comment.
We're unlikely to accept a big change that's just your personal style preferences.
|
|
||
|
|
||
| class Base: | ||
| class Base(ABC): |
There was a problem hiding this comment.
Making this an ABC may well have negative performance effects.
|
@tusharsadhwani @JelleZijlstra I took few of the slow testcases to profile for memory and check hot classes. |
- Added comments and Changelog - reverted additional changes
4 participants
Description
Reference : issue #5216
every token and AST node in a formatted file is one of these)
and cuts allocation overhead across the hot formatting path.
used_namesandfixers_applied), never read anywhere.r = r0 | r1Checklist - did you ...
--previewstyle, following the stability policy?CHANGES.mdif necessary?