Optimize Parameter inheritance #1605
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm not sure if it's completely minimized, but there aren't obvious places for improvement right now. For comparison on number of calls for just importing psyneulink: devel:
this branch:
|
|
This is a pretty drastic improvement! Great work! |
|
Yeah, looks like, for the most frequently call functions, there is 2x-3x improvement (if I’m reading the tables correctly) which, for setup should feel like a major improvement. Has it been merged yet?
jdc
On May 8, 2020, at 2:33 PM, SamKG <notifications@github.com<mailto:notifications@github.com>> wrote:
This is a pretty drastic improvement! Great work!
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#1605 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACSHKGDYSJIDH3Q4IKSCB23RQRF7LANCNFSM4M22OYMQ>.
|
If a Parameter is marked as _inherited=True, then it does not store its default value (for example) explicitly but rather inherits the value in real time from its closest non-inherited parent. This was implemented by deletion of the attribute and repeated getattr calls to find the correct inherited value. This commit implements storage of a reference to the closest uninherited parent, which is recalculated lazily in the uncommon event that the correct parent is changed (by explicitly setting the default value for a previously inherited parent, or by calling Parameter.reset() on an uninherited parent in some cases).
Mainly to avoid nested getattr calls for parsing and validation methods. These are not Parameter objects, and if they're present, they will be in the Parameters.__dict__ and accessible normally.
Avoids keeping any deleted Parameter classes from garbage collection
kmantel
force-pushed
the
refac/parameters/optimize
branch
from
388379e
to
e4a34af
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the need for most getattr calls using caching.
If a
Parameteris marked as_inherited=True, then it does not store its default value (for example) explicitly but rather inherits the value in real time from its closest non-inherited parent. This was implemented by deletion of the attribute and repeated getattr calls to find the correct inherited value.This branch implements storage of a reference to the closest uninherited parent, which is recalculated lazily in the uncommon event that the correct parent is changed (by explicitly setting the default value for a previously inherited parent, or by calling
Parameter.reset()on an uninherited parent in some cases).It also removes most getattr calls for
Parameters, which now mostly only occurrs when setting upParameterAliases