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.

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

Silence several Hint[Performance] warnings #23003

Merged
merged 4 commits into from
Nov 29, 2023

Conversation

c-blake
Copy link
Contributor

@c-blake c-blake commented Nov 29, 2023

With --mm:arc one gets the "implicit copy; if possible, rearrange your program's control flow" Performance warnings without these moves.

implicit copy; if possible, rearrange your program's control flow to
prevent it [Performance]` warnings that trigger without the `move`s.
Add one for `parseHeader` 's `parseList` instead.
@@ -234,7 +234,7 @@ func parseList(line: string, list: var seq[string], start: int): int =
while start+i < line.len and line[start + i] notin {'\c', '\l'}:
i += line.skipWhitespace(start + i)
i += line.parseUntil(current, {'\c', '\l', ','}, start + i)
list.add(current)
list.add(move current)
if start+i < line.len and line[start + i] == ',':
i.inc # Skip ,
current.setLen(0)
Copy link
Member

Choose a reason for hiding this comment

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

This line is now unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure that isn't pending "near-term/not yet merged" work? I still get this with HEAD of nim-devel compiled 15 minutes ago:

shell$ nim r --hint[Performance]=on --mm:orc tests/stdlib/thttpcore.nim
/usr/lib/nim/lib/pure/httpcore.nim(167, 25) Hint: passing 'headers.table[toCaseInsensitive(headers, key)]' to a sink parameter introduces an implicit copy; if possible, rearrange your program's control flow to prevent it [Performance]
/usr/lib/nim/lib/pure/httpcore.nim(237, 14) Hint: passing 'current' to a sink parameter introduces an implicit copy; if possible, rearrange your program's control flow to prevent it [Performance]
Hint: mm: orc; opt: none (DEBUG BUILD, `-d:release` generates faster code)
47489 lines; 0.641s; 71.609MiB peakmem; proj: thttpcore; out: ....

The line 237 one is the relevant warning, of course. (Not sure how to fix the line 167 one.) Anyway, happy to remove the line, if it really won't be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Well move current does set current to "" already and so the setLen 0 should not be required.

@c-blake
Copy link
Contributor Author

c-blake commented Nov 29, 2023

I don't see how the current CI failures relate to this PR. Maybe those failures are endemic lately?

@Araq Araq merged commit beeacc8 into nim-lang:devel Nov 29, 2023
16 of 19 checks passed
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from beeacc8

Hint: mm: orc; opt: speed; options: -d:release
177174 lines; 7.356s; 766.691MiB peakmem

@c-blake c-blake deleted the AddMovesToSilencePerfWarning branch June 13, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants