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

runtime: scavengeOne ignores searchIdx optimization #54892

Closed
mvdan opened this issue Sep 6, 2022 · 6 comments
Closed

runtime: scavengeOne ignores searchIdx optimization #54892

mvdan opened this issue Sep 6, 2022 · 6 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Sep 6, 2022

// [...]
// searchIdx is the page index to start searching from in ci.
// [...]
func (p *pageAlloc) scavengeOne(ci chunkIdx, searchIdx uint, max uintptr) uintptr {

Yet searchIdx has never been used in the func body, as far as I can see. Is the parameter truly unnecessary and can be removed, or is it a bug that the parameter isn't being used? I found this one via unparam, but it's not a clear one without understanding the context of the function.

cc @mknyszek @prattmic per git blame :)

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 6, 2022
@mvdan
Copy link
Member Author

mvdan commented Sep 6, 2022

Another one nearby: gcmarknewobject does not use its scanSize parameter, but at least that one isn't documented.

@mknyszek
Copy link
Contributor

mknyszek commented Sep 6, 2022

Ah, that's a mistake, albeit a minor one. It's not a bug, just slightly less efficient. It should be passed to findScavengeCandidate instead of pallocChunkPages-1.

@mknyszek mknyszek self-assigned this Sep 6, 2022
@mknyszek mknyszek added this to the Go1.20 milestone Sep 6, 2022
@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 6, 2022
@mknyszek mknyszek changed the title runtime: scavengeOne seems to have an unused parameter runtime: scavengeOne ignores searchIdx optimization Sep 6, 2022
@mknyszek
Copy link
Contributor

mknyszek commented Sep 6, 2022

The extra argument gcmarknewobject is likely a leftover from the Go 1.18 pacer updates. I believe it was used by the Go 1.17 pacer and when I deleted the branches in Go 1.19 I neglected to delete the parameter.

@mknyszek
Copy link
Contributor

mknyszek commented Sep 6, 2022

No need to file an issue about gcmarknewobject, I'll just clean that up sometime this cycle.

@mvdan
Copy link
Member Author

mvdan commented Sep 6, 2022

Ack, I'll leave both here for you then :)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/429255 mentions this issue: runtime: use searchIdx in scavengeOne

@golang golang locked and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants