Howto:Troubleshooting Nasal Callbacks: Difference between revisions
No edit summary |
|||
Line 1: | Line 1: | ||
== Motivation == | |||
{{FGCquote | |||
|1= Someone recently pointed out that the setlistener() / removelistener() API makes it easy to leak resources. So I wondered about making an alternate API where the return value from setlistener '''must''' be kept, or the listener is removed. I can imagine this with a helper object | |||
<syntaxhighlight lang="nasal"> | |||
var myL = setlistener2("some/prop", func { ... }} ) | |||
myL.addprop("some/other/prop"); | |||
myL.addprop("yet/another/prop"); | |||
</syntaxhighlight> | |||
Now you need to retain a ref to myL or the listeners on all the props are removed. I don't think we can retro-fit this to the existing API, because I suspect many places just ignore the return value and would break with this change. | |||
(Or even pass a vec of property names to setlistener2, to avoid all the discrete calls to 'addprop', that's a seperate detail really) | |||
It seems like this would work, and be easy enough to implement - question is if it gives enough benefit to be worth the confusion. | |||
|2= {{cite web | |||
| url = http://forum.flightgear.org/viewtopic.php?p=192767#p192767 | |||
| title = <nowiki>Listener objects</nowiki> | |||
| author = <nowiki>zakalawe</nowiki> | |||
| date = Oct 28th, 2013 | |||
}} | |||
}} | |||
== Background == | == Background == | ||
{{FGCquote | {{FGCquote | ||
Line 48: | Line 69: | ||
}} | }} | ||
{{FGCquote | {{FGCquote |
Revision as of 21:48, 31 January 2016
Motivation
Background
At EDXH, the Effect system attaches approx. 1500 Listeners to the property tree but only to 17 unique properties At KSFO, the Effect system attaches some 10000 Listeners to the property tree - but also only to 17 unique properties. That's with shader quality set toero (all off). — Torsten Dreyer (Sep 4th, 2014). Re: [Flightgear-devel] crash in SGPropertyNode::fireValueChanged.
(powered by Instant-Cquotes) |
On my old Laptop, (1.6GH dual core, GeForce Go 7400, 3Gb RAM) FlightGear has becom barely usable over the last few years. Framerates of max 20 at EDXH and single digits at KSFO. — Torsten Dreyer (Sep 4th, 2014). Re: [Flightgear-devel] crash in SGPropertyNode::fireValueChanged.
(powered by Instant-Cquotes) |
Without the patch, thousands of listeners were triggered each frame — Torsten Dreyer (Aug 29th, 2014). Re: [Flightgear-devel] crash in SGPropertyNode::fireValueChanged.
(powered by Instant-Cquotes) |
We've (hopefully successfully!) reduced the number of property listeners generated by the Effects system massively - there should now be only one per property. I'm hopeful that this may provide a performance boost for CPU-limited systems, as well as removing one crash source. — Stuart Buchanan (Sep 27th, 2014). Re: [Flightgear-devel] crash in SGPropertyNode::fireValueChanged.
(powered by Instant-Cquotes) |
usually, "events" means Nasal callbacks (timers) - however, it's also often not just Nasal per se, but misuse of timers - in general, settimer() is a API that is prone to being used improperly, because it needs to be consciously used, i.e. to enable/disable loops and to prevent them from being "registered" (invoked) too often - which is the most common pattern of misuse we've seen so far: Nasal based callbacks that end up being invoked doens (or even hundreds) of times despite being only intended to be invoked once per interval.
However, to be fair, this is not specific to Nasal - A few months ago, Torsten fixed pretty much the exact same bug in the C++ effects code which was registering thousands of identical callbacks. The underlying problem here is that the SGPropertyNode code does not currently support any form of troubleshooting, i.e. to do callback tracking - for details, refer to: Towards better troubleshooting The maketimer() API can be used to help prevent such issues when using timers, but listeners are a completely different matter (if in doubt, please file a feature request) |
To learn more about timers and how they work in Nasal, see $FG_SRC/Scripting/NasalSys.cxx
The settimer() API is mapped to the f_settimer() C function here: http://sourceforge.net/p/flightgear/flightgear/ci/next/tree/src/Scripting/NasalSys.cxx#l781 The C code for this extension function can be seen here: http://sourceforge.net/p/flightgear/flightgear/ci/next/tree/src/Scripting/NasalSys.cxx#l471 That's only 3 lines of code, which ends up calling the FGNasalSys::setTimer() API at: http://sourceforge.net/p/flightgear/flightgear/ci/next/tree/src/Scripting/NasalSys.cxx#l1265 |
What this will do is create a temporary NasalTimer object, for the class/struct refer to: http://sourceforge.net/p/flightgear/flightgear/ci/next/tree/src/Scripting/NasalSys.hxx#l163
It will then set up the NasalTimer object according to the arguments you passed to settimer() - and ultimately, it will invoke the event manager to register the timer object: http://sourceforge.net/p/flightgear/flightgear/ci/next/tree/src/Scripting/NasalSys.cxx#l1298 That really is all you need to know in order to check if a Nasal callback gets re-added, or if it is already running, because the "handler" parameter can be checked (e.g. use SG_LOG() to print out the address of the pointer). Most of this would be also applicable to the setlistener() API, with the difference being that calls will not be specific to any particular subsystem like "events", but can be triggered by arbitrary subsystems which end up writing/modifying a property with a registered Nasal callback/listener. These two are the most likely suspects when it comes to leaking callbacks and having redundant callbacks registered to be executed unnecessarily. Note that the event manager lives in SimGear, and one simple thing you can do is to log the number of active timers to the console and/or to the property tree (e.g. using a propertyObject) - you will almost certainly see those grow over time, probably even with identical callbacks: http://api-docs.freeflightsim.org/simgear/classSGEventMgr.html The other thing that can be done is to use SGTimeStamp::now() to benchmark the execution time of different callbacks. None of this would be specific to the 777 - it would benefit any/all aircraft and other Nasal code suffering from these coding errors
|
Objective
Implementation
We will be wrapping/overriding the problematic APIs at the global level to provide functions that internally support callback/registration tracking, and which can automatically detect improper use of these APIs:
var ObjectIdentity = func(obj) {
return id(obj);
};
# TODO: need to use call/compile to track call location
var TrackableResource = {
new: func() {
var m={parents:[TrackableResource]};
m.objects=[];
return m;
},
del: func() {
},
};
var TrackableAPI = {
new: func(api) {
var m = { parents:[TrackableAPI] };
m.collection = {};
return m;
},
del: func() {},
getCallback: func() {return me.invoke},
invoke: func (callback, interval) {
var key=id(callback);
if (me.collection[key]!=nil)
print("Warning, key/callback already in collection: ");
},
};
var setlistener = TrackableAPI.new(api: setlistener).getCallback();
# removelistener
var settimer = TrackableAPI.new(api: settimer).getCallback();
var test = func() {
## improper use of APIs:
var foo = func;
for (var i=0;i<=5;i++) {
# 5 listeners registered for the same property/callback:
setlistener("/sim/foo", foo);
# invoke foo again after 5 seconds
settimer(foo, 5);
} # for loop
}; # of test
Integration
The corresponding APIs should be either loaded globally via $FG_ROOT/Nasal or explicitly loaded via the -set.xml file: