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

Fix multi-level indirection in Parameters access #840

Merged
merged 1 commit into from
Sep 23, 2023
Merged

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Sep 23, 2023

Started looking at .param and noticed this really bizarre (and pointless) double indirection when accessing variables on the Parameters object. There's a bunch of cases where instead of accessing methods directly on the Parameters object we go through this two-level indirect attribute access that just to get back to the same object, i.e. self_.self_or_cls.param._TRIGGER simplifies to just self_._TRIGGER.

In fact this is so silly that fixing it gives a 10% performance boost for a bunch of common operations:

Screen Shot 2023-09-23 at 22 20 22

Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Lgtm. I'm not sure if these are the same bits where I was noticing a similar issue, but in that case I couldn't tell whether the indirection was because of trying to write code that works both with a class and with an instance, or whether it was just left over from originally moving code into the param object without updating it. In any case the solution is the same, i.e. make sure we have tests covering both cases and then it's fine. Thanks!

@philippjfr
Copy link
Member Author

philippjfr commented Sep 23, 2023

I'm not sure if these are the same bits where I was noticing a similar issue, but in that case I couldn't tell whether the indirection was because of trying to write code that works both with a class and with an instance, or whether it was just left over from originally moving code into the param object without updating it.

I'm pretty sure the indirection was always pointless and was introduced in an unnecessary attempt to make it work for both classes and instances, i.e. .self_or_cls.param by definition always takes you back to the same Parameters object you're already on.

@philippjfr philippjfr merged commit 23af556 into main Sep 23, 2023
10 checks passed
@philippjfr philippjfr deleted the fix_indirection branch September 23, 2023 20:33
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