From 3f6d8ab9b159d040905451af1cf5d913c38bbef6 Mon Sep 17 00:00:00 2001 From: David Rose Date: Thu, 9 Jul 2009 17:12:27 +0000 Subject: [PATCH] better object_id pointer passing --- direct/src/plugin/p3dPythonObject.cxx | 5 ++- direct/src/plugin/p3dPythonRun.cxx | 47 ++++++++++++++++++++++----- direct/src/plugin/p3dPythonRun.h | 8 +++++ direct/src/plugin/p3dSession.cxx | 32 ++++++++++++++---- direct/src/plugin/p3dSession.h | 7 ++++ 5 files changed, 83 insertions(+), 16 deletions(-) diff --git a/direct/src/plugin/p3dPythonObject.cxx b/direct/src/plugin/p3dPythonObject.cxx index 0d271ae2ed..0278b463d9 100644 --- a/direct/src/plugin/p3dPythonObject.cxx +++ b/direct/src/plugin/p3dPythonObject.cxx @@ -211,11 +211,14 @@ call(const string &method_name, P3D_object *params[], int num_params) const { TiXmlElement *xcommand = new TiXmlElement("command"); xcommand->SetAttribute("cmd", "pyobj"); xcommand->SetAttribute("op", "call"); - xcommand->SetAttribute("object_id", _object_id); if (!method_name.empty()) { xcommand->SetAttribute("method_name", method_name); } + TiXmlElement *xobject = _session->p3dobj_to_xml(this); + xobject->SetValue("object"); + xcommand->LinkEndChild(xobject); + for (int i = 0; i < num_params; ++i) { TiXmlElement *xparams = _session->p3dobj_to_xml(params[i]); xcommand->LinkEndChild(xparams); diff --git a/direct/src/plugin/p3dPythonRun.cxx b/direct/src/plugin/p3dPythonRun.cxx index f0e48acf7f..e5ac75ba61 100755 --- a/direct/src/plugin/p3dPythonRun.cxx +++ b/direct/src/plugin/p3dPythonRun.cxx @@ -32,6 +32,8 @@ P3DPythonRun(int argc, char *argv[]) { INIT_LOCK(_commands_lock); INIT_THREAD(_read_thread); + _next_sent_id = 0; + _program_name = argv[0]; _py_argc = 1; _py_argv = (char **)malloc(2 * sizeof(char *)); @@ -321,9 +323,10 @@ handle_pyobj_command(TiXmlElement *xcommand, bool needs_response, } else if (strcmp(op, "call") == 0) { // Call the named method on the indicated object, or the object // itself if method_name isn't given. - int object_id; - if (xcommand->QueryIntAttribute("object_id", &object_id) == TIXML_SUCCESS) { - PyObject *obj = (PyObject *)(void *)object_id; + TiXmlElement *xobject = xcommand->FirstChildElement("object"); + if (xobject != NULL) { + PyObject *obj = xml_to_pyobj(xobject); + const char *method_name = xcommand->Attribute("method_name"); // Build up a list of params. @@ -433,6 +436,8 @@ handle_pyobj_command(TiXmlElement *xcommand, bool needs_response, xresponse->LinkEndChild(pyobj_to_xml(result)); Py_DECREF(result); } + + Py_DECREF(obj); } } } @@ -989,12 +994,25 @@ pyobj_to_xml(PyObject *value) { // This is more expensive for the caller to deal with--it requires // a back-and-forth across the XML pipe--but it's much more // general. - // TODO: pass pointers better. - xvalue->SetAttribute("type", "python"); - xvalue->SetAttribute("object_id", (int)(intptr_t)value); - // TODO: fix this hack, properly manage these reference counts. + int object_id = _next_sent_id; + ++_next_sent_id; + bool inserted = _sent_objects.insert(SentObjects::value_type(object_id, value)).second; + while (!inserted) { + // Hmm, we must have cycled around the entire int space? Either + // that, or there's a logic bug somewhere. Assume the former, + // and keep looking for an empty slot. + object_id = _next_sent_id; + ++_next_sent_id; + inserted = _sent_objects.insert(SentObjects::value_type(object_id, value)).second; + } + + // Now that it's stored in the map, increment its reference count. + // TODO: implement removing things from this map. Py_INCREF(value); + + xvalue->SetAttribute("type", "python"); + xvalue->SetAttribute("object_id", object_id); } return xvalue; @@ -1053,12 +1071,23 @@ xml_to_pyobj(TiXmlElement *xvalue) { } else if (strcmp(type, "python") == 0) { int object_id; if (xvalue->QueryIntAttribute("object_id", &object_id) == TIXML_SUCCESS) { - return (PyObject *)(void *)object_id; + SentObjects::iterator si = _sent_objects.find(object_id); + PyObject *result; + if (si == _sent_objects.end()) { + // Hmm, the parent process gave us a bogus object ID. + result = _undefined; + } else { + result = (*si).second; + } + Py_INCREF(result); + return result; } } // Something went wrong in decoding. - return Py_BuildValue(""); + PyObject *result = _undefined; + Py_INCREF(result); + return result; } //////////////////////////////////////////////////////////////////// diff --git a/direct/src/plugin/p3dPythonRun.h b/direct/src/plugin/p3dPythonRun.h index 8d83d3095c..1bee044a1d 100755 --- a/direct/src/plugin/p3dPythonRun.h +++ b/direct/src/plugin/p3dPythonRun.h @@ -117,6 +117,14 @@ private: PT(GenericAsyncTask) _check_comm_task; + // This map keeps track of the PyObject pointers we have delivered + // to the parent process. We have to hold the reference count on + // each of these until the parent process tells us it's safe to + // release them. + typedef pmap SentObjects; + SentObjects _sent_objects; + int _next_sent_id; + // The remaining members are manipulated by the read thread. typedef pdeque Commands; Commands _commands; diff --git a/direct/src/plugin/p3dSession.cxx b/direct/src/plugin/p3dSession.cxx index f18ef1a045..74724c11bd 100644 --- a/direct/src/plugin/p3dSession.cxx +++ b/direct/src/plugin/p3dSession.cxx @@ -41,6 +41,8 @@ P3DSession(P3DInstance *inst) { _session_key = inst->get_session_key(); _python_version = inst->get_python_version(); + _next_sent_id = 0; + _p3dpython_running = false; _next_response_id = 0; _response = NULL; @@ -359,7 +361,13 @@ xml_to_p3dobj(const TiXmlElement *xvalue) { } else if (strcmp(type, "browser") == 0) { int object_id; if (xvalue->QueryIntAttribute("object_id", &object_id) == TIXML_SUCCESS) { - P3D_object *obj = (P3D_object *)object_id; + SentObjects::iterator si = _sent_objects.find(object_id); + if (si == _sent_objects.end()) { + // Hmm, the child process gave us a bogus object ID. + return new P3DUndefinedObject; + } + + P3D_object *obj = (*si).second; return P3D_OBJECT_COPY(obj); } @@ -371,7 +379,7 @@ xml_to_p3dobj(const TiXmlElement *xvalue) { } // Something went wrong in decoding. - return new P3DNoneObject; + return new P3DUndefinedObject; } //////////////////////////////////////////////////////////////////// @@ -434,13 +442,25 @@ p3dobj_to_xml(const P3D_object *obj) { // Otherwise, it must a host-provided object, which means we // should pass a reference down to this particular object, so // the Python process knows to call back up to here to query it. - // TODO: pass pointers better. Fix this hideous leak. + P3D_object *dup = P3D_OBJECT_COPY(obj); - int object_id = (int)(intptr_t)dup; + + int object_id = _next_sent_id; + ++_next_sent_id; + bool inserted = _sent_objects.insert(SentObjects::value_type(object_id, dup)).second; + while (!inserted) { + // Hmm, we must have cycled around the entire int space? Either + // that, or there's a logic bug somewhere. Assume the former, + // and keep looking for an empty slot. + object_id = _next_sent_id; + ++_next_sent_id; + inserted = _sent_objects.insert(SentObjects::value_type(object_id, dup)).second; + } + + // TODO: implement removing things from this map. + xvalue->SetAttribute("type", "browser"); xvalue->SetAttribute("object_id", object_id); - - // TODO: manage this reference count somehow. } break; } diff --git a/direct/src/plugin/p3dSession.h b/direct/src/plugin/p3dSession.h index a36b7fd31b..9e00c071be 100644 --- a/direct/src/plugin/p3dSession.h +++ b/direct/src/plugin/p3dSession.h @@ -107,6 +107,13 @@ private: typedef vector Commands; Commands _commands; + // This map keeps track of the P3D_object pointers we have delivered + // to the child process. We have to keep each of these until the + // child process tells us it's safe to delete them. + typedef map SentObjects; + SentObjects _sent_objects; + int _next_sent_id; + P3DPackage *_panda3d; PackageCallback *_panda3d_callback;