GNOME Bugzilla – Bug 116183
GOK does needless API calls when logging not enabled
Last modified: 2011-10-14 11:00:32 UTC
GOK does a lot of API calls (particularly to Accessible_getName) for the purposes of providing log messages. IN many cases (particularly in gok_spy_accessible_implicit_ref()) these calls happen even if logging is not enabled. This is a performance hit we don't need to take. I've patched the above problem away in CVS, but I think we need to look at the issue of logging in general and make sure that we're not calling API unnecessarily when GOK is running in "normal" mode. Also I think making logging a compile-time option is not as nice as making it a runtime option, but that's a different bug :-) Lastly, do we really need gok_spy_accessible_implicit_ref in the first place? It seems like a diagnostic tool to me, perhaps it should be a runtime option.
Good thinking and nice recent commits! The diagnostic functions have proven very useful in the past. If we started #ifdef'ing I suggest we do so in one place, and use MACRO expansion otherwise the code becomes unecessarily complex. Can we do this in ANSI C?
Apologies for spam... marking as GNOMEVER2.3 so it appears on the official GNOME bug list :)
David: I would go for a combination of (1) converting debug mode from compile-time to runtime where feasible; (2) moving redundant/reused code to functions that can be stubbed-out if debugging is not turned on (3) creating some macros as you suggest. Probably requires a nice thorough slog through the code, but probably not more than a day's work.
Bill, you mentioned you still wanted this bug - but that was a while ago. Is that still the case? I think it makes sense for you to own it since you have put thought into to the fix.
I had a look at the code and noticed that as long as HAVE_ISO_C99_VARIADIC or HAVE_GNU_VARIADIC is defined, gok won't make the log calls if logging is not specified at build time. The calls to gok_spy_accessible_ref and gok_spy_accessible_implicit_ref aren't really a big deal. I'll add a patch to make them even less so. I'm downgrading the priority on this one.
Created attachment 24034 [details] [review] simplifies gok spy ref calls when not logging
putting priority back up. David, I am seeing CORBA calls when logging is not enabled that ought to be purely diagnostic. I am not sure why the macro args are being expanded, but they apparently are.
BTW gok-spy patch looks good, please commit.
Its committed - thanks for the review. I am intrigued about the corba calls, are you convinced they come about from needless API calls? Are you certain that macro expansion is behind this?
Bill rightly reverted this patch - a better one is coming.
Created attachment 24067 [details] fixed patch
I don't think this patch would have any performance impact to speak of; just counter incrementing etc. is IMO not worth chasing. We have bigger fish to fry :) I thought there were still some at-spi calls happenning when logging is not enabled...
"We have bigger fish to fry :)" Agreed! If I see any needless API calls I'll toss a patch here. (That said, this patch should probably be applied since counters etc. are only relevant if logging is enabled.)
Bill are you still seeing those "CORBA calls" when logging is not enabled?
I believe so, but I'd need to investigate to give specifics. One thing that we often do is call at-spi API and pass the params to gok_log(). I think it might make sense to revise the gok-log API a bit so that instead of just replicating fprintf, we passed enum values into gok_log to correspond to the fprintf format string args. For instance we might pass gok_log a format string, a log level, an accessible-object (or NULL), and an srgv array corresponding to the format string. Then: gok_log (1, "object %s [%s]has %d children", object, GOK_LOG_OBJECT_NAME, GOK_LOG_OBJECT_ROLE, GOK_LOG_OBJECT_CHILDCOUNT); might mean log level 1, and GOK_LOG_OBJECT_NAME would mean "pass Acceessible_getName (object) as param 1 of the format string", etc. etc. This means of course that if gok_log evaluates to no-op, these atk calls don't happen, but we don't have to use #ifdef build-time brackets to prevent the calls. This would also facilitate runtime control of gok_log (including different log levels) which would be ab improvement. Lastly, instead of log levels we could use flags, thus gok_log (GOK_LOG_UI_SEARCH | GOK_LOG_EVENTS, "....
removed patch keyword. David, what do you think of my recent proposal above, which would deal with macro expansion issues and argument expansion into gok_log calls?
I think having log levels/flags/categories would be very useful. I think maybe your suggestions belong as a separate RFE though... ?
This is the best way I see for eliminating the 'needless API calls' since the remaining costly calls are the ones which provide the gok_log macros their parameters. So I think this really is the same issue, this is my best shot at solving it.
marking AP4, keeping normal priority; the need to fix this bug is mostly internal to GOK.
Apologies for spam-- ensuring Sun a11y team are cc'ed on all current a11y bugs. Filter on "SUN A11Y SPAM" to ignore.
gok (GNOME on-screen keyboard) development has been stalled and it has been replaced by caribou [1]. Maintainers don't have future development plan so i am closing all the bugs as WONTFIX. [1] https://mail.gnome.org/archives/gnome-bugsquad/2011-October/msg00001.html