Bug 27459 - implement a fast-path for Connection properties from spec 0.19.2
Summary: implement a fast-path for Connection properties from spec 0.19.2
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Olli Salli
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/og...
Whiteboard: NB#213334
Keywords: patch
Depends on: 27513
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-05 04:48 UTC by Simon McVittie
Modified: 2011-04-12 03:38 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Simon McVittie 2010-04-05 04:48:27 UTC
telepathy-spec 0.19.2 added Connection.Status, Connection.Interfaces properties to Connection (a SelfHandle property already existed).

Our client bindings should use GetAll(CONNECTION) as a fast path, falling back to the separate accessor methods to support old CMs.
Comment 1 Simon McVittie 2010-04-26 05:08:47 UTC
This might be a little bit subtle, but shouldn't be too hard to do.
Comment 2 Olli Salli 2011-04-07 12:58:56 UTC
It wasn't totally easy! Have a look at my branch (URL).

Namely, when developing that I stumbled upon
 * A race condition in contact building, uncovered by the awesome performance of GetAll introspection
 * Several tests relying on the individual slow-path introspection calls being made with a specific timing
 * TpConnection being a maze of inter-connected callbacks with a lot of corner cases (to be expected, really)

lcov-check indicates that both the fast and slow paths are hit for both the pre-connected and connected phases of introspection, and also as a response to StatusChanged.
Comment 3 Olli Salli 2011-04-10 02:20:50 UTC
As a side note, this actually produces a measurable performance benefit!

with master:

make -j check  5.60s user 0.93s system 63% cpu 10.359 total

with oggis/conn-introspection

make -j check  5.44s user 0.92s system 62% cpu 10.189 total    

So the CPU time taken for running the test suite decreases by about 2.9% and the wallclock time by about 1.6%, although a few tests still deliberately hit the old slow-path to test corner-cases for it. The timings also include a lot of Makefile crunching, so the relative tp-glib speedup is actually greater.
Comment 4 Will Thompson 2011-04-11 06:26:17 UTC
http://git.collabora.co.uk/?p=user/oggis/telepathy-glib.git;a=commitdiff;h=f4514c86c30c2cfa5a18de84918af5c8079a96c6

Doesn't TpDBusPropertiesMixin crash (or at least critical) if getting the GObject property fails?

http://git.collabora.co.uk/?p=user/oggis/telepathy-glib.git;a=commitdiff;h=222bfe4ee068d08c633457906610299f4c2d8eba

I think I'd split the grabbing-things-from-properties block into its own function, so that you can just return FALSE; when things go awry; this gets rid of the goto and would make it easier to skim-read.

Test-wise, I guess that the existing tests implicitly exercise the new code paths, and the tests where you've deliberately broken the properties test the fallback?
Comment 5 Olli Salli 2011-04-11 09:48:19 UTC
(In reply to comment #4)
> http://git.collabora.co.uk/?p=user/oggis/telepathy-glib.git;a=commitdiff;h=f4514c86c30c2cfa5a18de84918af5c8079a96c6
> 
> Doesn't TpDBusPropertiesMixin crash (or at least critical) if getting the
> GObject property fails?
> 

No :>

> http://git.collabora.co.uk/?p=user/oggis/telepathy-glib.git;a=commitdiff;h=222bfe4ee068d08c633457906610299f4c2d8eba
> 
> I think I'd split the grabbing-things-from-properties block into its own
> function, so that you can just return FALSE; when things go awry; this gets rid
> of the goto and would make it easier to skim-read.

I did.

I also split out the bit of the code which adds the interfaces to the TpProxy as tp_proxy_add_interfaces. One more bit of API which we'll have to add to 0.14 :P

> 
> Test-wise, I guess that the existing tests implicitly exercise the new code
> paths, and the tests where you've deliberately broken the properties test the
> fallback?

Yes.
Comment 6 Olli Salli 2011-04-11 10:10:32 UTC
The branch conn-introspection-0.14 is da same thing backported to 0.14.
Comment 7 Will Thompson 2011-04-12 02:40:47 UTC
+ * Declare that this proxy supports the given interfaces. Equivalent to calling
+ * g_quark_from_string () followed by tp_proxy_add_interface () for each of the
+ * interface names.

tp_proxy_add_interface() doesn't exist; you mean tp_proxy_add_interface_by_id().

This branch is going to fail make check with gtk-doc enabled, because you didn't add tp_proxy_add_interfaces to telepathy-glib-sections.txt.

I'd kind of expect tp_proxy_add_interfaces to early-return if interfaces == NULL.

Fix the above, and then merge the -0.14 variant to 0.14 please!
Comment 8 Olli Salli 2011-04-12 03:38:07 UTC
(In reply to comment #7)
> + * Declare that this proxy supports the given interfaces. Equivalent to
> calling
> + * g_quark_from_string () followed by tp_proxy_add_interface () for each of
> the
> + * interface names.
> 
> tp_proxy_add_interface() doesn't exist; you mean
> tp_proxy_add_interface_by_id().
> 

True! Fixed.

> This branch is going to fail make check with gtk-doc enabled, because you
> didn't add tp_proxy_add_interfaces to telepathy-glib-sections.txt.

Oh. I knew I had to have noobed something with gtk-doc. Fixed, and make check with --enable-gtk-doc verified to pass.

> 
> I'd kind of expect tp_proxy_add_interfaces to early-return if interfaces ==
> NULL.

Fair enough. It does now.

> 
> Fix the above, and then merge the -0.14 variant to 0.14 please!

Thanks! Done. I leave the forward-porting runes to get it into master to you, though.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.