Board index FlightGear Development Nasal

Listener objects

Nasal is the scripting language of FlightGear.

Listener objects

Postby zakalawe » Mon Oct 28, 2013 4:11 pm

Dumb question of the day. 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

var myL = setlistener2("some/prop", func { ... }} )
myL.addprop("some/other/prop");
myL.addprop("yet/another/prop");

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.
zakalawe
 
Posts: 1259
Joined: Sat Jul 19, 2008 5:48 pm
Location: Edinburgh, Scotland
Callsign: G-ZKLW
Version: next
OS: Mac

Re: Listener objects

Postby Hooray » Mon Oct 28, 2013 4:24 pm

That should work, pretty much analogous to how SGTimer is being wrapped via maketimer() now.
IIRC, some folks are simply overloading setlistener() at the submodule level - I think that's what Thorsten's local weather system is doing: wrapping setlistener() and using an implementation that simply calls the global setlistener() while storing return values in a vector, that is getting freed in a foreach loop if necessary.

Regarding "leaking", I think it should be technically possible to identify listeners that are still active but which have no active references and issue a corresponding warning at runtime, Philosopher can probably comment on the details here - but in general, we only need to check if a listener id that's getting freed points to an active listener.

However, for new users, it might be easier to support an additional/optional argument in setlistener, which determines the scope of the listener (aircraft, gui, sim etc) - and then keep track of listener IDs internally (in a vector) and free it upon receiving the corresponding sim signal.
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Listener objects

Postby Philosopher » Mon Oct 28, 2013 6:31 pm

Sounds good to me – I think I already thought of this idea privately. There might be an issue with closures, though: the function for the setlistener cannot be GCSave'd (aka unconditional referencing), it must be marked when the listener that references it is marked. This would require exposing a marking API to ghosts in addition to a reaping/collection API.

Hooray wrote in Mon Oct 28, 2013 4:24 pm:Philosopher can probably comment on the details here - but in general, we only need to check if a listener id that's getting freed points to an active listener.

Yeah, the problem is that those ids are numbers and thus aren't kept track of and correspondingly gc'ed. Changing it to a simple ghost object or something would work, but that's back to the idea above, just with maintaining the old API. We could even just enable it as a "debugging" mode, and print a warning upon removal of a stale listener.
Philosopher
 
Posts: 1593
Joined: Sun Aug 12, 2012 7:29 pm

Re: Listener objects

Postby Hooray » Mon Oct 28, 2013 6:52 pm

the problem is that those ids are numbers and thus aren't kept track of and correspondingly gc'ed. Changing it to a simple ghost object or something would work

I haven't looked at the code in question, but maybe a simple workaround would be maintaining the existing API that uses integers, but internally using ghosts in a std::map<unsigned int, naFunc> ?
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Listener objects

Postby zakalawe » Mon Oct 28, 2013 7:04 pm

Philosopher wrote in Mon Oct 28, 2013 6:31 pm:Changing it to a simple ghost object or something would work, but that's back to the idea above, just with maintaining the old API. We could even just enable it as a "debugging" mode, and print a warning upon removal of a stale listener.

Right, I wondered about this, but my concern is backwards compatibility, that's why I proposed a new name with different semantics.
zakalawe
 
Posts: 1259
Joined: Sat Jul 19, 2008 5:48 pm
Location: Edinburgh, Scotland
Callsign: G-ZKLW
Version: next
OS: Mac

Re: Listener objects

Postby Hooray » Mon Oct 28, 2013 8:01 pm

Having some form of "debug" mode that provides feedback whenever such "leaking listeners" are encountered would indeed be a good idea, and could also help when reviewing new aircraft or other scripts for fgdata prior to accepting them for inclusion.
On the other hand, one of the more common resource usage issues are Nasal callbacks that are accidentally executed more than once due to poor scripting, no matter if it's via timers or listeners - usually, script-specific main loops etc are only intended to be run once - but we've been seeing lots of issues related to this, including poor frame rates because of callbacks being invoked over and over again, despite the fact that they were already running, without anybody ever noticing the culprit here - unless they know how to debug/troubleshoot Nasal code.

So ideally, we would have some form of "singleton concept", so that users could directly specify if a callback is supposed/safe to be run "concurrenly", or if it should only be fired once, and the C++ code would help identify such occurences.

See for reference: http://www.mail-archive.com/flightgear- ... 36668.html
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Listener objects

Postby Hooray » Sun Nov 10, 2013 4:20 pm

some more feedback based on reworking the ND.nas code:

another useful thing would be if such listener objects could be aware of tied properties and show error messages if someone tries to "listen" to tied properties.
also, some properties are written at frame rate - where getprop-polling would probably be better.

the SGTimer/maketimer() API would ideally also allow for a callback to be specified in case framerate/performance is too bad to run the timer at the interval specified.
sort of like a notification callback that would get called if necessary - that would standardize the various feature-scaling attempts we have, i.e. down-grading features according to frame rate/latency.

setlistener/settimer are way too low-level for most fgdata contributors, because they require manual management and explicit design to prevent certain bugs/issues, such as having identical callbacks running concurrently, even though the programmer really only wanted a single instance of a callback/loop to be active.

I would also think an option to automatically release listeners/timer objects during sim-reinit would be useful - this should probably be enabled by default.
That would probably solve 90% of timer/listener issues we have in fgdata.

If these could be implemented, I would argue that the setlistener() API should be re-implemented on top of this, so that listeners are implicitly tracked/managed in case the developer forgets about it - _setlistener() itself would remain as is for more sophisticated/low-level needs.
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Listener objects

Postby Hooray » Sat Feb 22, 2014 11:49 pm

given the recent number of "performance" debates we've had, I looked into adding additional diagnostics to FGNasalSys, and I found quite a number of "rogue" scripts that are running via listeners/timers, where aircraft developers are obviously not even aware of what is going on under the hood. So, personally, I am all in favor of deprecating the low-level APIs like setlistener/settimer and enforcing such callbacks to be registered via objects, so that additional meta information can be more easily saved (file, line number), but also to help enforce not having multiple instances of a callback - which seems a very common issue (also pointed out by ThorstenB a while ago). In addition, I would even extend such objects so that they must provide a "signal handler" so that FlightGear can terminate objects by sending a signal to kill off the loop and run the exit-handler - that would allow us to come up with a simple "process monitor" where we could immediately see how many "background processes" (=Nasal callbacks) are running, where they were created (file, line number, optional description) - and we could even send them a termination signal.

In turn, we would remove setlistener/settimer access and require people to use objects, thoughts / ideas ?
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Listener objects

Postby Hooray » Mon Feb 24, 2014 11:30 pm

okay, thanks to James' recent draw-mask commit, I've been able to test a few aircraft without any scenery/models/aircraft being rendered, and while the OSG stats were looking pretty good/flat, there's other stuff going on, such as proprules & input consuming CPU time for non-apparent reasons according to the system monitor. Now, with regard to Nasal callbacks, those are not trivial to disable/measure usually without rebuilding FGNasalSys and wrapping SGTimer/SGPropertyChangeListener stuff there.

So I was thinking we could refine maketimer/makelistener by allowing "scopes" to be specified (e.g. core, system, gui, scenery, aircraft) - it would then be trivial to use a different queue for each "scope", so that the "aircraft" queue could be suspended by not processing any callbacks with the corresponding scope set. That should help address all Nasal related concerns, because aircraft developers could easily verify if their Nasal code is having a massive impact or not.

Besides, we could then also ensure that aircraft Nasal scripts cannot directly access globals.maketimer/globals.settimer/globals.setlistener, possibly even wrapping those to automatically create properly-scoped timer/listener-callbacks that always have aircraft-scope.
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Listener objects

Postby Hooray » Thu Feb 27, 2014 1:55 am

continuing the monologue to add that it would make sense to simply extend the event manager class -at least for timers- because all timers (settimer & maketimer) already have an internal "name" field that is set to the pointer of the callback - in other words, we could simply add const char* file/line stuff there to easily keep track of all timers and the location where they got invoked from (file name, line number), ideally with some configurable "identifier", such as "pfd update loop" - which should greatly help us with troubleshooting, and determining where some Nasal callback came from in the first place...

To handle listeners, we'd want to add the same meta info to FGNasalListener

In turn, that would allow us to easily come up with a list of running timer/listeners callbacks, as well as their origin - we could even use a counter to keep track of the number of loops running that way.
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Listener objects

Postby Philosopher » Thu Feb 27, 2014 2:19 am

Naming sounds good - I think keeping track of that in Nasal would be relatively easy. Plus, then we can use the Nasal console to look up active timers/listeners ;).
Philosopher
 
Posts: 1593
Joined: Sun Aug 12, 2012 7:29 pm

Re: Listener objects

Postby Hooray » Thu Feb 27, 2014 2:24 am

I am thinking in terms of having an implicit "name" consisting of file/line number where the timer got invoked, which is available (i.e. see naRuntimeError()) - but also of explicit naming, so that people can (eventually should/must) specify identifiers for their callbacks - preferably, even specifying if their callbacks are allowed to run interleaved with other instances of the same callback or not - that would allow FG to provide basic diagnostics whenever someone makes one of the most common mistakes - i.e. re-registering callbacks over and over again (even if just during reset/re-init).
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Listener objects

Postby Hooray » Sun Mar 09, 2014 4:45 pm

It seems, that Issue #1403 is also related to rogue Nasal callbacks that are re-registered and running after multiple reposition iterations, without anybody noticing it:

Hooray wrote:
Philosopher wrote:Bug report from Hyde: http://code.google.com/p/flightgear-bug ... il?id=1403 :? Listeners, timers? Or C++ problems?

James recently activated his reset/re-init code, so that's probably what's causing this - and he seems busy investigating all bug reports, so I won't interfere unless this won't be solved within 1-2 weeks

Without having checked anything, I suppose it's related to callbacks still running, either property listeners, or timers - which is what used to cause such issues previously. One more reason, why we should investigate having an "active" callback list and exposing it to Nasal. Then again, as far as I know, James is shutting down all of Nasal, including listeners/timers. But that's the "low-hanging fruit" that I'd check first
..........
ok, it's interesting: it doesn't even need to be a different airport, I just used a bit of Nasal in a foreach loop to reposition at KSFO, and at around ~30 iterations, the whole sessions starts becoming dead-slow.

My previous comments were not quite correct though - because this is just "reposition", and not reset/reinit.

But even after 60 iterations, I am not seeing a crash here
But framerate is now single-digits and latency is ~2000 ms

So, should be easy to troubleshoot using the fgcommand-profiler
Ok, it's running now here: 100 reposition fgcommands, and one profiler-start fgcommand to see what's going on there
..........
seems like my original assumption wasn't too far off - according to the system monitor, it's "events" that's taking up all the CPU time, which is where Nasal timers (setlistener/maketimer) are registered.
---------
this shouldn't be solved within the next few days, I'll have another look - but it seems that exposing the size of the timer queue and printing it to the terminal should provide some insight: https://gitorious.org/fg/simgear/source ... gr.hxx#L65

............
confirmed, we were spot-on: I patched simgear's event manager to dump the size of the queues, and after ~30 resets, things are starting to grow massively, I am seeing rougly 160 active timers vs. just 15 by default

OnceI hit ~500 active timers, things are becoming real slow, i.e. 2000ms, but active timers start being "freed", and according to the profiler, the GC is kinda busy here, too

It takes roughly 60 seconds before the number of active timers is back to under 100 again

And according to pointer checks, there's multiple instances of a single callback running now - so it matches the whole scope of our discussion in the Nasal forum pretty well ... too bad that nobody would listem :?

But even after 5 minutes, things will not get back to ~15 timers, i.e. there remain ~80 active instances running
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Listener objects

Postby Hooray » Mon Mar 17, 2014 6:37 am

Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Listener objects

Postby Hooray » Fri Aug 29, 2014 4:15 pm

Update:
Hooray wrote in Mon Feb 24, 2014 11:30 pm:while the OSG stats were looking pretty good/flat, there's other stuff going on, such as proprules & input consuming CPU time for non-apparent reasons according to the system monitor. Now, with regard to Nasal callbacks, those are not trivial to disable/measure usually without rebuilding FGNasalSys and wrapping SGTimer/SGPropertyChangeListener stuff there.


Torsten now seems to have tracked down the issue related to property rules:

http://sourceforge.net/p/flightgear/mai ... /32774953/
Torsten wrote:As a side effect, I have noticed that the load for the xml-property rule
subsystem has decreased on my system
significantly. This subsystem generates values for most of the
environment properties responsible of the
instantiated effects. Without the patch, thousands of listeners were
triggered each frame resulting in an average
of 1ms execution time per frame for the proprule subsystem. With the
patch, this has decreased to an average
of 0.07ms per frame. This is on a 3.5GHz 6 Core Xeon. Weaker machines
might benefit even more.
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU


Return to Nasal

Who is online

Users browsing this forum: No registered users and 6 guests