-
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 all 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 |
|---|---|---|
|
|
@@ -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) | ||
|
|
||
|
|
||
|
|
||
| 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.
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. Yes, its redundant parenthesis hence removed. |
||
|
|
||
| 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. |
||
| 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]) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this reduce memory usage? Python won't allocate a new
1object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is mainly a readability/simplicity change, not a memory optimization.