-
Notifications
You must be signed in to change notification settings - Fork 85
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
Refactor ListenerParser to be a plain old Python class #1490
Conversation
Here are some informal timings on my machine, showing an approximate 80% speedup on a micro-benchmark. First, on this branch (leaving out the steps that failed because I mistyped something):
And the same timing on the main branch:
|
I realised I was using the wrong IPython for one of those timings (the global IPython instead of the one in the venv). Here are new timings, still showing more or less (actually, more rather than less) the same improvement:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM with one small question.
Also, it'd be useful to mention what performance problem this PR is addressing. I think it's the performance of text parsing in on_trait_change
handlers but it'd be nice to document it in the PR. Also, I think the profiling work was driven by #1489 , right?
@@ -1009,9 +993,18 @@ def _get_name(self): | |||
|
|||
# -- object Method Overrides ---------------------------------------------- | |||
|
|||
def __init__(self, text="", **traits): | |||
def __init__(self, text): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not a 100% sure why we're changing the text
default here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just for safety.
Previously, the text
in the ListenerParser
could be updated after initialization, triggering the _text_changed
listener. With the refactored code, the only way the ListenerParser
can be used correctly is to provide the text
directly at initialization time. I made it non-optional so that we catch any code that tries to create a ListenerParser
without providing the text to parse.
I added a note to the description. |
This PR refactors the
ListenerParser
to turn it into a plain old Python class, rather than aHasTraits
class.This was mostly motivated by performance; this is only a small part of the performance story, but it is a part, and it's easy to fix. The overall goal is to speed up the creation of Traits listeners, since that was proving a bottleneck in some large Traits-using applications. This PR speeds up parsing of listener text strings like "foo.bar[]".
The behaviour of the old class also didn't make a lot of sense: in the
__init__
method, there was a lineself.text = text
, prior to calling the superclass__init__
. Thatself.text = text
assignment triggered the_text_changed
method, which then did all the parsing. That meant that all of the parsing had already taken place, and thelistener
trait set, before thesuper().__init__(**traits)
call took place, making that superclass call entirely redundant. The trait-change dispatch to the_text_changed
method was showing up as significant (not huge, but significant) in profiling.This was motivated by #1489: we still have a lot of old Delegate-based code around (particularly in TraitsUI), and so speeding up that code should provide a quick win for downstream code.