wx.NewId fails with too many IDs

[smk78]

ESS Win 7 v3.1.0 Build 124

See attached screenshot.

Was fitting some data, had data/model & residual plots for each of 3 data sets. All was fine. Then checked all three data sets in the Data Explorer and clicked on New Plot (because I wanted to see what the data looked like on semi-log axes). A new graph window appeared and the data was plotted but none of the icons or the graph (right-click) context menu worked. Same problem if you click in any of the existing graphs.

Thereafter, if you only select one dataset and try and send it to fitting you get a popup error:

Fitting set_data: C++ assertion "(itemid ≥ 0 && itemid < SHRT_MAX)

(itemid ≥ wxID_AUTO_LOWEST && itemid ⇐ wxID_AUTO_HIGHEST)" failed at ..\..\src\common\menucmn.cpp(260) in wxMenuItemBase::wxMenuItemBase(): invalid itemid value

Basically, from this point on SasView? is to all extents and purposes useless.

[smk78]

What I think is happening is that SasView? is asking wxWidgets to assign a graph widget an invalid id.

Hunting through the forums it seems that itemid is ONLY passed as a low-word, even in 32-bit applications, meaning you can only have 32767 id's (UNLESS you allow negative id's as well!).

I had been generating several graphs when the error occurred AND I had been experimenting with DREAM, which, of course, generates it's own plots.

So my theory is that something about wx3.0, and/or what we are doing now in our graph handling, has pushed us closer to the id limit then we have been in the past. Interestingly I note on the wxWidget website that wx3.0.0 was notable in that a lot of wxWidget work had gone into it.

If my theory is correct, then the only SasView? users likely to be affected are those doing intensive graph operations. On that basis I will downgrade the status of this ticket from blocker to critical.

[ajj]

This is failing in menucmn.cpp from wx. Thus the problem is that a new menu item has an wxID outside the allowed range.

I believe we use wx.NewID() to generate wxIDs everywhere, so presumably this is using negative values too. The problem is not limited to graphs - you must have hit a point where there are more than 2^{16 WX object having been instantiated simultaneously.}

Actually … looking into it further, it seems that windows only allows positive numbers for menu IDs, so 2^{15 wx objects must have been created.}

wx.NewID() generated ids sequentially from some minimum value (below that are reserved for standard IDs) up to a maximum and then wraps round. So deos this mean that the wxPython bit of the code is not doing a check on the value of the IDs being generated?

​http://docs.wxwidgets.org/trunk/overview_windowids.html

[ajj]

No, wxPython just directly wraps the wxWidgets code, so the answer seems to be that wxNewId in wxWidgets doesn't do any checking … I guess that is why the menu code has to check.

We probably need to catch this and then set the ID ourselves somehow …

Bit worrying that we have generated over 32k IDs though.

Reference:

See : ​https://github.com/wxWidgets/wxWidgets/blob/master/src/common/utilscmn.cpp

The wxNewID() function does the following:

——————————————————————————————————————
Id functions
——————————————————————————————————————

Id generation
static int wxCurrentId = 100;

int wxNewId()
{

skip the part of IDs space that contains hard-coded values:
if (wxCurrentId == wxID_LOWEST)

wxCurrentId = wxID_HIGHEST + 1;

return wxCurrentId++;

}

i.e. it does no checking at all and just allows the value to wrap once it overflows.

[butler]

This is a much bigger problem than we can deal with here as it will require significant refactoring effort. Further this has always been a problem and only pops up if somebody does a LOT of fitting/plotting. leave as know issue.

[pkienzle]

I dropped the following code into sasview.py after “import wx” so I could see where NewId?() was being called:

import inspect
_NewId = wx.NewId?
def NewId?():

id = _NewId()
frame = inspect.stack()[1]
path = frame[1]
index = path.find('/sas/')
if index == -1: index = path.find('\\sas\\')
path = path[index+1:]
print "NewId? %d from %s(%d):%s"%(id, path, frame[2], frame[3])
return id

Wx.NewId? = NewId?

Unfortunately, this doesn’t show the source of the big leak of 10 ids per second, but it will be helpful in finding where we are creating ids over and over again.

To see evidence of the leak, choose >Tools > Python Shell/Editor? from the menu and enter the following:

import time, wx
t0,n0 = time.time(), wx.NewId?()
t1,n1 = time.time(), wx.NewId?(); print t1-t0, n1, (n1-n0)/(t1-t0)

It turns out that inside the idletimer is a wx timer that takes an id as input. Modify gui_manager.ViewerFrame?._redraw_idle to contain:

print “timer id”, self.idletimer.timer.GetId?()

And you can see it count up quickly when the app has focus and slowly when it doesn’t.

Fixing this will require creating our own copy of the wx CallAfter? class for which we can set the id ourselves.

[ajj]

For those interested, the relevant wx code is here:

​http://svn.wxwidgets.org/viewvc/wx/wxPython/trunk/src/_core_ex.py?view=markup

Our idletimer is a call to wx.CallLater?, which itself calls wx.CallAfter?:

def CallAfter(callableObj, *args, **kw):
    """
    Call the specified function after the current and pending event
    handlers have been completed.  This is also good for making GUI
    method calls from non-GUI threads.  Any extra positional or
    keyword args are passed on to the callable when it is called.

    :see: `wx.CallLater`
    """
    assert callable(callableObj), "callableObj is not callable"
    app = wx.GetApp()
    assert app is not None, 'No wx.App created yet'

    if not hasattr(app, "_CallAfterId"):
        app._CallAfterId = wx.NewEventType()
        app.Connect(-1, -1, app._CallAfterId,
                    lambda event: event.callable(*event.args, **event.kw) )
    evt = wx.PyEvent()
    evt.SetEventType(app._CallAfterId)
    evt.callable = callableObj
    evt.args = args
    evt.kw = kw
    wx.PostEvent(app, evt)

Would setting _CallAfterId on the app object to define the event type avoid the repeated generation of IDs from wx.NewEventType?()?

I can't test this myself at the moment because my Mac doesn't currently build sasview

[ajj]

To answer my own question. No. Setting _CallAfterId does not solve the problem.

[ajj]

Problem appears to be in wx.CallLater?.

Timer is generated using wx.PyTimer?, which does not take an id, but derived its ID from the default constructor of wx.Timer which uses ID_ANY.

We might be able to solve this by providing our own version of wx.CallLater? that is derived from wx.Timer

Though it seems that this should be a problem lots of people have, so it likely means we are abusing wx.CallLater? and should actually change the way we manage plot updates etc.

[butler]

Given the nature of this bug I think we have to move it into release 4.0 unless there are objections?

[Paul Kienzle <pkienzle@…>]

In [6f3fea20af1759130054b5e4cfa4a251dc06cf52]:

workaround for NewId? creation in timer loop. Refs #448

[Paul Kienzle <pkienzle@…>]

In [2d88fc4a6cc278c2ea7ac05c75e83d324432f287]:

reduce number of NewIds? used for each plot. Refs #448.

[pkienzle]

The most egregious leaks are now plugged, but there will still be a slow leakage of IDs over time as graphs are opened and closed. This can be fixed by creating a pool of IDs, where the graph, etc. asks the pool for an id when it needs one and releases it to the pool when it is done.

This is a low priority task which can be put off to a future release, or perhaps indefinitely.

[Paul Kienzle <pkienzle@…>]

In [d7b4decd86aa380c3097056f40899305200dd6ff]:

fix constraint Remove button. Fixes #468. Refs #448.

[butler]

As we are moving to Qt with version 5.0, "indefinite" may be now: It will be irrelevant at that point and clearly as a low priority anyway no longer useful to even consider. Will close as won't fix (only because technically it is not yet "obsolete")