GNOME Bugzilla – Bug 502924
Only ref() event members if they are still referenced after the callback returns
Last modified: 2021-07-05 12:36:49 UTC
Event callbacks receive references to a number of remote accessible objects through an Event structure. The remote objects are not ref()ed because the are only "borrowed" by the callback, if the AT wishes to use any of the remote objects at a later date after the callback returns, they must be ref()ed. In the status quo the remote objects are reffed unconditionally. This introduces a certain traffic overhead since every time an event callback is called, even if it is empty the member objects are implicitly ref()ed and unref()ed.
Created attachment 100713 [details] [review] Proposed changes This patch determines weather a reference is being heldby the remote objects after the callbacks return. If there are still references, it does a ref(). This patch depends on a pyorbit weakref support patch found in bug 502925.
Created attachment 103125 [details] [review] Proposed changes This patch gets rid of the weakref scheme, and simply uses sys.getrefcout instead.
Comment on attachment 103125 [details] [review] Proposed changes Some a general questions: What happens if the user has given async=True to Registry.start()? Alternatively, what happens if the client is doing some for of decoupling where all events received are stored for later dispatch to avoid blocking the bridge? In both of these cases, the Python ref count *all* events received will increase by 1 by design, regardless of whether they are used later or not. But what about the accessibles attached to the events? Just storing an event doesn't increase the ref count of the accessibles automatically does it? So will these accessibles be improperly discarded while the event is being held in the dispatch queue? >+ >+ # Record the current reference count of the event object and it's >+ # remote-object structure members. >+ prev_refs = {'event': sys.getrefcount(ev)} >+ for attrib in ('source', 'any_data', 'host_application'): >+ refrencable = getattr(ev, attrib, None) >+ if isinstance(refrencable, Accessibility.Accessible): >+ refrencable.dont_unref = True >+ prev_refs[attrib] = sys.getrefcount(refrencable) I worry a bit about the performance of creating a dictionary, looping, and hashing for every event received. It might be worth exploring some optimizations like unrolling the loop, keeping separate vars for the four known fields, etc. Need to profile. >+ # If there is an extra reference to the event object we need to ref() >+ # all the member objects. >+ ref_all = sys.getrefcount(ev) - prev_refs['event'] > 0 >+ >+ # If there is an extra reference to any remote object - ref() it. >+ for attrib in prev_refs: >+ refrencable = getattr(ev, attrib, None) >+ if refrencable is not None: >+ if ref_all or \ >+ sys.getrefcount(refrencable) - prev_refs[attrib] > 0: >+ refrencable.ref() >+ refrencable.dont_unref = False Again, some performance concerns about iterating for every event. More importantly, the question about storing the Python event, but never explicitly touching its accessible member variables applies here. Will they be properly ORBit ref counted? >- SLOTS = ('_icache', '_property_cache', '_user_data') >+ SLOTS = ('_icache', '_property_cache', '_user_data', 'dont_unref') dont_unref should start with a _ like the other properties.
(In reply to comment #3) > What happens if the user has given async=True to Registry.start()? > Alternatively, what happens if the client is doing some for of decoupling where > all events received are stored for later dispatch to avoid blocking the bridge? > In both of these cases, the Python ref count *all* events received will > increase by 1 by design, regardless of whether they are used later or not. Yup, if async is True or if the client is doing it's own decoupling then we don't get any of the "borrowing" benefit. And the objects are always ref()ed. The only time when this will be a possible optimization is if the client is doing the decoupling, but it is also dropping events that don't meet certain criteria, maybe if the event.hot_application is not the right app. > But what about the accessibles attached to the events? Just storing an event > doesn't increase the ref count of the accessibles automatically does it? So > will these accessibles be improperly discarded while the event is being held > in the dispatch queue? If the event instance is stored then all of the remote object members will be ref()ed: ref_all = sys.getrefcount(ev) - prev_refs['event'] > 0 > It might be worth exploring some > optimizations like unrolling the loop, keeping separate vars for the four known > fields, etc. Need to profile. Yeah, that shouldn't be a problem, we could just unroll it. Overall the question is does this add a layer of complexity that will get us in trouble later, or maybe all the extra logic makes it not worthwhile for performance. > More > importantly, the question about storing the Python event, but never explicitly > touching its accessible member variables applies here. Will they be properly > ORBit ref counted? > I believe so. I ran a few automated Orca tests and the output (braille/speech) results are identical with or without the patch. I also tested this by storing every other event and seeing if it ref()ed everything it needed to. It did. > dont_unref should start with a _ like the other properties. I figured not to prefix it with _ because it was a public attribute. I think it is ugly in general, I would like to find a more elegant solution.
[Resetting QA Contact to newly introduced "at-spi-maint@gnome.bugs". Reason: So far it was impossible to watch changes in at-spi bug reports without following all the specific persons (Li Yuan, Bill Haneman, Jeff Wai, ...) and also their activity outside of at-spi reports. IMPORTANT: Anyone interested in following all bug activity (including all maintainers) must watch the "at-spi-maint@gnome.bugs" dummy user by adding it to the 'Users to watch' list under Preferences->Email preferences. This is also the default procedure nowadays in GNOME when setting up new products.]
[Mass-resetting default assignee, see bug 705890. Please reclaim this bug report by setting the assignee to yourself if you still plan to work on this. Thanks!]
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/pyatspi2/-/issues/ Thank you for your understanding and your help.