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

Unsused argument in _PyCode_Quicken #128872

Open
WolframAlph opened this issue Jan 15, 2025 · 7 comments
Open

Unsused argument in _PyCode_Quicken #128872

WolframAlph opened this issue Jan 15, 2025 · 7 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage type-feature A feature request or enhancement

Comments

@WolframAlph
Copy link
Contributor

WolframAlph commented Jan 15, 2025

_PyCode_Quicken in specialize.c accepts PyObject *consts but does not use it. Not sure which type of issue to use, so creating blank one.

Linked PRs

@picnixz
Copy link
Member

picnixz commented Jan 15, 2025

Instead of removing them, I think it's better to simply mark consts with a Py_UNUSED attribute (namely Py_UNUSED(consts)). The compiler could also optimize the call and the diff would be smaller. If we need to add it later for some reason, it's probably better that way.

However, just adding Py_UNUSED() is also not needed as the compiler could be already smart enough. Do we gain something with this change on macro and micro benchmarks?

@picnixz picnixz added type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Jan 15, 2025
@WolframAlph
Copy link
Contributor Author

WolframAlph commented Jan 15, 2025

There is no difference in terms of generated machine code. As expected, compiler is smart enough to ignore it. But still, should it be left like this?

@picnixz
Copy link
Member

picnixz commented Jan 15, 2025

If there is no difference, no need to change this. We rejected previous PRs that wanted to add Py_UNUSED() consistently. I don't think we want a successful build with -Wunused-parameter (otherwise there are many other places to change). It doesn't hurt to add it if _PyCode_Quicken() is changed, but otherwise, I'm not really convinced.

@WolframAlph
Copy link
Contributor Author

Closing it then

@picnixz picnixz closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2025
@Eclips4
Copy link
Member

Eclips4 commented Jan 15, 2025

_PyCode_Quicken is a purely internal API. So, I guess someone from the faster-cpython team should decide whether we should remove it or not. Maybe this parameter will be useful in the future.
cc @markshannon as author of the ddd9599 which removed the use of consts argument.

@brandtbucher
Copy link
Member

If the arg is unused, let's remove it. The fact that it was left was just an oversight of that PR.

@brandtbucher brandtbucher reopened this Jan 16, 2025
@WolframAlph
Copy link
Contributor Author

Reopened PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants