-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Memory Optimizations #5222
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.
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
base: main
Are you sure you want to change the base?
Memory Optimizations #5222
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,7 @@ | |
| LN = Union[Leaf, Node] | ||
|
|
||
|
|
||
| @dataclass | ||
| @dataclass(slots=True, eq=False) | ||
| class Line: | ||
| """Holds leaves and comments. Can be printed with `str(line)`.""" | ||
|
|
||
|
|
@@ -199,9 +199,7 @@ def _is_triple_quoted_string(self) -> bool: | |
| value = self.leaves[0].value | ||
| if value.startswith(('"""', "'''")): | ||
| return True | ||
| if value.startswith(("r'''", 'r"""', "R'''", 'R"""')): | ||
| return True | ||
| return False | ||
| return value.startswith(("r'''", 'r"""', "R'''", 'R"""')) | ||
|
rsb-23 marked this conversation as resolved.
Outdated
|
||
|
|
||
| @property | ||
| def is_docstring(self) -> bool: | ||
|
|
@@ -483,22 +481,21 @@ def __str__(self) -> str: | |
| return "\n" | ||
|
|
||
| indent = " " * self.depth | ||
| leaves = iter(self.leaves) | ||
| first = next(leaves) | ||
| res = f"{first.prefix}{indent}{first.value}" | ||
| res += "".join(str(leaf) for leaf in leaves) | ||
| comments_iter = itertools.chain.from_iterable(self.comments.values()) | ||
| comments = [str(comment) for comment in comments_iter] | ||
| res += "".join(comments) | ||
| first, *rest = self.leaves | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new code overall seems nicer here but note this materializes
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks to point it out. |
||
|
|
||
| rest_str = "".join(map(str, rest)) | ||
| comments_str = "".join( | ||
| str(comment) for group in self.comments.values() for comment in group | ||
| ) | ||
|
|
||
| return res + "\n" | ||
| return f"{first.prefix}{indent}{first.value}{rest_str}{comments_str}\n" | ||
|
|
||
| def __bool__(self) -> bool: | ||
| """Return True if the line has leaves or comments.""" | ||
| return bool(self.leaves or self.comments) | ||
|
|
||
|
|
||
| @dataclass | ||
| @dataclass(slots=True, eq=False, repr=False) | ||
| class RHSResult: | ||
| """Intermediate split result from a right hand split.""" | ||
|
|
||
|
|
@@ -509,7 +506,7 @@ class RHSResult: | |
| closing_bracket: Leaf | ||
|
|
||
|
|
||
| @dataclass | ||
| @dataclass(slots=True, eq=False, repr=False) | ||
| class LinesBlock: | ||
| """Class that holds information about a block of formatted lines. | ||
|
|
||
|
|
@@ -549,7 +546,7 @@ class _DecoratedFuncInfo(NamedTuple): | |
| is_multi: bool | ||
|
|
||
|
|
||
| @dataclass | ||
| @dataclass(slots=True, eq=False, repr=False) | ||
| class EmptyLineTracker: | ||
| """Provides a stateful method that returns the number of potential extra | ||
| empty lines needed before and after the currently processed line. | ||
|
|
@@ -1610,7 +1607,6 @@ def can_omit_invisible_parens( | |
| # a leading opening bracket and a trailing closing bracket. If the | ||
| # opening bracket doesn't match our rule, maybe the closing will. | ||
|
|
||
| penultimate = line.leaves[-2] | ||
| last = line.leaves[-1] | ||
|
|
||
| if ( | ||
|
|
@@ -1624,6 +1620,7 @@ def can_omit_invisible_parens( | |
| and last.parent.type != syms.trailer | ||
| ) | ||
| ): | ||
| penultimate = line.leaves[-2] | ||
| if penultimate.type in OPENING_BRACKETS: | ||
| # Empty brackets don't help. | ||
| return False | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,8 +89,7 @@ def diff(a: str, b: str, a_name: str, b_name: str) -> str: | |
| if line[-1] == "\n": | ||
| diff_lines.append(line) | ||
| else: | ||
| diff_lines.append(line + "\n") | ||
| diff_lines.append("\\ No newline at end of file\n") | ||
| diff_lines.extend((line + "\n", "\\ No newline at end of file\n")) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making an intermediate tuple likely means more memory usage.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both append and extend makes a method lookup, so 1 is generally better. Also, it won't add to memory usage as Cython uses cache memory for tuple even if there's no tuple. |
||
| return "".join(diff_lines) | ||
|
|
||
|
|
||
|
|
@@ -113,9 +112,7 @@ def color_diff(contents: str) -> str: | |
|
|
||
|
|
||
| def style_output(message: str, **styles: Any) -> str: | ||
| if not _color_enabled(): | ||
|
rsb-23 marked this conversation as resolved.
|
||
| return message | ||
| return style(message, **styles) | ||
| return style(message, **styles) if _color_enabled() else message | ||
|
|
||
|
|
||
| def _color_enabled() -> bool: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -360,7 +360,7 @@ def _get_key(string: str) -> CustomSplitMapKey: | |
| A unique identifier that is used internally to map @string to a | ||
| group of custom splits. | ||
| """ | ||
| return (id(string), string) | ||
| return id(string), string | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This definitely doesn't affect memory usage. |
||
|
|
||
| def add_custom_splits( | ||
| self, string: str, custom_splits: Iterable[CustomSplit] | ||
|
|
@@ -422,6 +422,8 @@ class StringMerger(StringTransformer, CustomSplitMapMixin): | |
| StringMerger provides custom split information to StringSplitter. | ||
| """ | ||
|
|
||
| __slots__ = () | ||
|
|
||
| def do_match(self, line: Line) -> TMatchResult: | ||
| LL = line.leaves | ||
|
|
||
|
|
@@ -1016,13 +1018,11 @@ def do_transform( | |
| string_parser = StringParser() | ||
| rpar_idx = string_parser.parse(LL, string_idx) | ||
|
|
||
| should_transform = True | ||
| for leaf in (LL[string_idx - 1], LL[rpar_idx]): | ||
| if line.comments_after(leaf): | ||
| # Should not strip parentheses which have comments attached | ||
| # to them. | ||
| should_transform = False | ||
| break | ||
| # Should not strip parentheses which have comments attached | ||
| # to them. | ||
| should_transform = not any( | ||
| line.comments_after(leaf) for leaf in (LL[string_idx - 1], LL[rpar_idx]) | ||
| ) | ||
| if should_transform: | ||
| string_and_rpar_indices.extend((string_idx, rpar_idx)) | ||
|
|
||
|
|
@@ -1358,7 +1358,7 @@ def iter_fexpr_spans(s: str) -> Iterator[tuple[int, int]]: | |
| j = stack.pop() | ||
| # we've made it back out of the expression! yield the span | ||
| if not stack: | ||
| yield (j, i + 1) | ||
| yield j, i + 1 | ||
| i += 1 | ||
| continue | ||
|
|
||
|
|
@@ -1397,11 +1397,13 @@ def _toggle_fexpr_quotes(fstring: str, old_quote: str) -> str: | |
| escaping, once Black figures out how to parse the new grammar. | ||
| """ | ||
| new_quote = "'" if old_quote == '"' else '"' | ||
| parts = [] | ||
| parts: list[str] = [] | ||
| previous_index = 0 | ||
| 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(( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. old code is better
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're unlikely to accept a big change that's just your personal style preferences.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a style preference, but a micro-optimization. |
||
| fstring[previous_index:start], | ||
| fstring[start:end].replace(old_quote, new_quote), | ||
| )) | ||
| previous_index = end | ||
| parts.append(fstring[previous_index:]) | ||
| return "".join(parts) | ||
|
|
@@ -2257,10 +2259,7 @@ def do_transform( | |
| insert_str_child = insert_str_child_factory(LL[string_idx]) | ||
|
|
||
| comma_idx = -1 | ||
| ends_with_comma = False | ||
| if LL[comma_idx].type == token.COMMA: | ||
| ends_with_comma = True | ||
|
|
||
| ends_with_comma = LL[comma_idx].type == token.COMMA | ||
| leaves_to_steal_comments_from = [LL[string_idx]] | ||
| if ends_with_comma: | ||
| leaves_to_steal_comments_from.append(LL[comma_idx]) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.