From 47a03c33972a4eb0a73503cda6db755aefeffa30 Mon Sep 17 00:00:00 2001 From: Ykkrosh Date: Tue, 25 May 2010 18:24:12 +0000 Subject: [PATCH] Replace JS_Enumerate with manual enumeration, to avoid its memory allocations. Use LookupProperty to avoid having to check for getters. Add a quicker method of rooting many script values. This was SVN commit r7579. --- .../simulation/components/test-serialize.js | 2 +- source/scriptinterface/AutoRooters.cpp | 45 ++++++++++ source/scriptinterface/AutoRooters.h | 29 +++++++ source/scriptinterface/ScriptInterface.cpp | 23 ++++- source/scriptinterface/ScriptInterface.h | 5 +- .../serialization/BinarySerializer.cpp | 87 ++++++++++--------- .../serialization/BinarySerializer.h | 15 +++- .../simulation2/tests/test_ComponentManager.h | 3 +- 8 files changed, 158 insertions(+), 51 deletions(-) create mode 100644 source/scriptinterface/AutoRooters.cpp diff --git a/binaries/data/mods/_test.sim/simulation/components/test-serialize.js b/binaries/data/mods/_test.sim/simulation/components/test-serialize.js index 01a635097c..be8cba461d 100644 --- a/binaries/data/mods/_test.sim/simulation/components/test-serialize.js +++ b/binaries/data/mods/_test.sim/simulation/components/test-serialize.js @@ -66,7 +66,7 @@ function TestScript1_getter() {} TestScript1_getter.prototype.Init = function() { this.x = 100; - this.__defineGetter__('x', function () { return 200; }); + this.__defineGetter__('x', function () { print("FAIL\n"); die(); return 200; }); }; Engine.RegisterComponentType(IID_Test1, "TestScript1_getter", TestScript1_getter); diff --git a/source/scriptinterface/AutoRooters.cpp b/source/scriptinterface/AutoRooters.cpp new file mode 100644 index 0000000000..51ec4f3f9f --- /dev/null +++ b/source/scriptinterface/AutoRooters.cpp @@ -0,0 +1,45 @@ +/* Copyright (C) 2010 Wildfire Games. + * This file is part of 0 A.D. + * + * 0 A.D. is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 of the License, or + * (at your option) any later version. + * + * 0 A.D. is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with 0 A.D. If not, see . + */ + +#include "precompiled.h" + +#include "AutoRooters.h" + +#include "scriptinterface/ScriptInterface.h" + +AutoGCRooter::AutoGCRooter(ScriptInterface& scriptInterface) + : m_ScriptInterface(scriptInterface) +{ + m_Previous = m_ScriptInterface.ReplaceAutoGCRooter(this); +} + +AutoGCRooter::~AutoGCRooter() +{ + AutoGCRooter* r = m_ScriptInterface.ReplaceAutoGCRooter(m_Previous); + debug_assert(r == this); // must be correctly nested +} + +void AutoGCRooter::Trace(JSTracer* trc) +{ + if (m_Previous) + m_Previous->Trace(trc); + + for (size_t i = 0; i < m_Objects.size(); ++i) + { + JS_CALL_OBJECT_TRACER(trc, m_Objects[i], "AutoGCRooter object"); + } +} diff --git a/source/scriptinterface/AutoRooters.h b/source/scriptinterface/AutoRooters.h index 0e00a454b5..9dd2488524 100644 --- a/source/scriptinterface/AutoRooters.h +++ b/source/scriptinterface/AutoRooters.h @@ -15,8 +15,13 @@ * along with 0 A.D. If not, see . */ +#ifndef INCLUDED_AUTOROOTERS +#define INCLUDED_AUTOROOTERS + #include "js/jsapi.h" +class ScriptInterface; + // Exception-safety and GC-safety wrapper for JSIdArray // (TODO: it'd be nicer to use the existing js::AutoIdArray but currently that // requires a load of semi-private SpiderMonkey headers that cause a load of compile warnings) @@ -41,3 +46,27 @@ public: } }; +/** + * Helper for rooting large groups of script values. + * Construct this object, push values into it, and they will all be rooted until this + * object is destroyed. + * Many of these objects can be used at once, but their lifetimes must be correctly nested. + */ +class AutoGCRooter +{ +public: + AutoGCRooter(ScriptInterface& scriptInterface); + ~AutoGCRooter(); + + void Push(JSObject* obj) { m_Objects.push_back(obj); } + + void Trace(JSTracer* trc); +private: + ScriptInterface& m_ScriptInterface; + AutoGCRooter* m_Previous; + + std::vector m_Objects; + // TODO: add vectors of other value types +}; + +#endif // INCLUDED_AUTOROOTERS diff --git a/source/scriptinterface/ScriptInterface.cpp b/source/scriptinterface/ScriptInterface.cpp index e6f63af694..8cc72fdcd2 100644 --- a/source/scriptinterface/ScriptInterface.cpp +++ b/source/scriptinterface/ScriptInterface.cpp @@ -59,6 +59,8 @@ struct ScriptInterface_impl JSContext* m_cx; JSObject* m_glob; // global scope object JSObject* m_nativeScope; // native function scope object + + AutoGCRooter* m_rooter; }; namespace @@ -163,7 +165,16 @@ static void* jshook_function(JSContext* cx, JSStackFrame* fp, JSBool before, JSB } #endif -ScriptInterface_impl::ScriptInterface_impl(const char* nativeScopeName, JSContext* cx) +void jshook_trace(JSTracer* trc, void* data) +{ + ScriptInterface_impl* m = static_cast(data); + + if (m->m_rooter) + m->m_rooter->Trace(trc); +} + +ScriptInterface_impl::ScriptInterface_impl(const char* nativeScopeName, JSContext* cx) : + m_rooter(NULL) { JSBool ok; @@ -201,6 +212,8 @@ ScriptInterface_impl::ScriptInterface_impl(const char* nativeScopeName, JSContex | JSOPTION_VAROBJFIX // "recommended" (fixes variable scoping) ); + JS_SetExtraGCRoots(m_rt, jshook_trace, this); + // Threadsafe SpiderMonkey requires that we have a request before doing anything much JS_BeginRequest(m_cx); @@ -296,6 +309,14 @@ bool ScriptInterface::RemoveRoot(void* ptr) return JS_RemoveRoot(m->m_cx, ptr) ? true : false; } +AutoGCRooter* ScriptInterface::ReplaceAutoGCRooter(AutoGCRooter* rooter) +{ + debug_assert(m->m_rt); // this class must own the runtime, else the rooter won't work + AutoGCRooter* ret = m->m_rooter; + m->m_rooter = rooter; + return ret; +} + ScriptInterface::LocalRootScope::LocalRootScope(JSContext* cx) : m_cx(cx) { diff --git a/source/scriptinterface/ScriptInterface.h b/source/scriptinterface/ScriptInterface.h index 89506a87da..c11f9d969c 100644 --- a/source/scriptinterface/ScriptInterface.h +++ b/source/scriptinterface/ScriptInterface.h @@ -29,6 +29,8 @@ #include "ScriptTypes.h" #include "ScriptVal.h" +class AutoGCRooter; + namespace boost { class rand48; } // Set the maximum number of function arguments that can be handled @@ -37,7 +39,6 @@ namespace boost { class rand48; } #define SCRIPT_INTERFACE_MAX_ARGS 6 struct ScriptInterface_impl; -class ScriptClass; class ScriptInterface { public: @@ -181,6 +182,8 @@ public: bool AddRoot(void* ptr, const char* name); bool RemoveRoot(void* ptr); + AutoGCRooter* ReplaceAutoGCRooter(AutoGCRooter* rooter); + /** * Dump some memory heap debugging information to stderr. */ diff --git a/source/simulation2/serialization/BinarySerializer.cpp b/source/simulation2/serialization/BinarySerializer.cpp index f356b2ad88..fdcd998221 100644 --- a/source/simulation2/serialization/BinarySerializer.cpp +++ b/source/simulation2/serialization/BinarySerializer.cpp @@ -29,17 +29,14 @@ #include "scriptinterface/ScriptInterface.h" #include "scriptinterface/AutoRooters.h" +#include "js/jsobj.h" + CBinarySerializer::CBinarySerializer(ScriptInterface& scriptInterface) : - m_ScriptInterface(scriptInterface) + m_ScriptInterface(scriptInterface), m_ScriptBackrefsNext(1), m_Rooter(scriptInterface) { } -CBinarySerializer::~CBinarySerializer() -{ - FreeScriptBackrefs(); -} - /* The Put* implementations here are designed for subclasses @@ -159,45 +156,60 @@ void CBinarySerializer::HandleScriptVal(jsval val) } // Find all properties (ordered by insertion time) - JSIdArray* ida = JS_Enumerate(cx, obj); - if (!ida) - { - LOGERROR(L"JS_Enumerate failed"); - throw PSERROR_Serialize_ScriptError(); - } - // For safety, root all the property IDs - // (This should be unnecessary if we're certain that properties could never - // get deleted during deserialization) - IdArrayWrapper idaWrapper(cx, ida); - NumberU32_Unbounded("num props", (uint32_t)ida->length); + // JS_Enumerate is a bit slow (lots of memory allocation), so do the enumeration manually + // (based on the code from JS_Enumerate, using the probably less stable JSObject API): - for (jsint i = 0; i < ida->length; ++i) + // (Note that we don't do any rooting, because we assume nothing is going to trigger GC. + // I'm not absolute certain that's necessarily a valid assumption.) + + jsval iter_state, num_properties; + if (!obj->enumerate(cx, JSENUMERATE_INIT, &iter_state, &num_properties)) + throw PSERROR_Serialize_ScriptError("enumerate INIT failed"); + debug_assert(JSVAL_TO_INT(num_properties) >= 0); + size_t n = (size_t)JSVAL_TO_INT(num_properties); + + // Note: num_properties might be 0 if the object doesn't know in advance how many to enumerate; + // we can't distinguish that from really having 0 properties without performing the actual iteration, + // so just assume the object always returns the correct count + + NumberU32_Unbounded("num props", (uint32_t)n); + + jsid id; + + for (size_t i = 0; i < n; ++i) { + if (!obj->enumerate(cx, JSENUMERATE_NEXT, &iter_state, &id)) + throw PSERROR_Serialize_ScriptError("enumerate NEXT failed"); + + if (JSVAL_IS_NULL(iter_state)) + throw PSERROR_Serialize_ScriptError("enumerate NEXT gave unexpected null"); + jsval idval, propval; - // Forbid getters, because they will be weird and not serialise properly - JSPropertyDescriptor descr; - if (!JS_GetPropertyDescriptorById(cx, obj, ida->vector[i], JSRESOLVE_QUALIFIED, &descr)) - throw PSERROR_Serialize_ScriptError("JS_GetPropertyDescriptorById failed"); - if (descr.attrs & JSPROP_GETTER) - throw PSERROR_Serialize_ScriptError("Cannot serialize property getters"); - // Get the property name as a string - if (!JS_IdToValue(cx, ida->vector[i], &idval)) + if (!JS_IdToValue(cx, id, &idval)) throw PSERROR_Serialize_ScriptError("JS_IdToValue failed"); JSString* idstr = JS_ValueToString(cx, idval); if (!idstr) throw PSERROR_Serialize_ScriptError("JS_ValueToString failed"); - CScriptValRooted idstrRoot(cx, STRING_TO_JSVAL(idstr)); ScriptString("prop name", idstr); - if (!JS_GetPropertyById(cx, obj, ida->vector[i], &propval)) - throw PSERROR_Serialize_ScriptError("JS_GetPropertyById failed"); + // Use LookupProperty instead of GetProperty to avoid the danger of getters + // (they might delete values and trigger GC) + if (!JS_LookupPropertyById(cx, obj, id, &propval)) + throw PSERROR_Serialize_ScriptError("JS_LookupPropertyById failed"); HandleScriptVal(propval); } + + // Check we really reached the end of the iteration + if (!obj->enumerate(cx, JSENUMERATE_NEXT, &iter_state, &id)) + throw PSERROR_Serialize_ScriptError("enumerate NEXT failed"); + if (!JSVAL_IS_NULL(iter_state)) + throw PSERROR_Serialize_ScriptError("enumerate NEXT didn't give unexpected null"); + break; } case JSTYPE_FUNCTION: @@ -273,7 +285,7 @@ u32 CBinarySerializer::GetScriptBackrefTag(JSObject* obj) // The tags are stored in a map. Maybe it'd be more efficient to store it inline in the object // somehow? but this works okay for now - std::pair::iterator, bool> it = m_ScriptBackrefs.insert(std::make_pair(obj, (u32)m_ScriptBackrefs.size()+1)); + std::pair it = m_ScriptBackrefs.insert(std::make_pair(obj, m_ScriptBackrefsNext)); // If it was already there, return the tag if (!it.second) @@ -281,19 +293,8 @@ u32 CBinarySerializer::GetScriptBackrefTag(JSObject* obj) // If it was newly inserted, we need to make sure it gets rooted // for the duration that it's in m_ScriptBackrefs - if (!JS_AddRoot(m_ScriptInterface.GetContext(), (void*)&it.first->first)) - throw PSERROR_Serialize_ScriptError("JS_AddRoot failed"); + m_Rooter.Push(it.first->first); + m_ScriptBackrefsNext++; // Return a non-tag number so callers know they need to serialize the object return 0; } - -void CBinarySerializer::FreeScriptBackrefs() -{ - std::map::iterator it = m_ScriptBackrefs.begin(); - for (; it != m_ScriptBackrefs.end(); ++it) - { - if (!JS_RemoveRoot(m_ScriptInterface.GetContext(), (void*)&it->first)) - throw PSERROR_Serialize_ScriptError("JS_RemoveRoot failed"); - } - m_ScriptBackrefs.clear(); -} diff --git a/source/simulation2/serialization/BinarySerializer.h b/source/simulation2/serialization/BinarySerializer.h index d5aa35bd94..2d2730f77e 100644 --- a/source/simulation2/serialization/BinarySerializer.h +++ b/source/simulation2/serialization/BinarySerializer.h @@ -20,6 +20,8 @@ #include "ISerializer.h" +#include "scriptinterface/AutoRooters.h" + #include /** @@ -31,7 +33,6 @@ class CBinarySerializer : public ISerializer NONCOPYABLE(CBinarySerializer); public: CBinarySerializer(ScriptInterface& scriptInterface); - virtual ~CBinarySerializer(); protected: virtual void PutNumber(const char* name, uint8_t value); @@ -48,10 +49,16 @@ private: // PutScriptVal implementation details: void ScriptString(const char* name, JSString* string); void HandleScriptVal(jsval val); - u32 GetScriptBackrefTag(JSObject* obj); - void FreeScriptBackrefs(); - std::map m_ScriptBackrefs; + ScriptInterface& m_ScriptInterface; + + typedef std::map backrefs_t; + + backrefs_t m_ScriptBackrefs; + u32 m_ScriptBackrefsNext; + u32 GetScriptBackrefTag(JSObject* obj); + + AutoGCRooter m_Rooter; }; #endif // INCLUDED_BINARYSERIALIZER diff --git a/source/simulation2/tests/test_ComponentManager.h b/source/simulation2/tests/test_ComponentManager.h index ab0a7b8a9c..662b833fe5 100644 --- a/source/simulation2/tests/test_ComponentManager.h +++ b/source/simulation2/tests/test_ComponentManager.h @@ -707,7 +707,8 @@ public: man.AddComponent(ent1, man.LookupCID("TestScript1_getter"), noParam); std::stringstream stateStream; - TS_ASSERT_THROWS_PSERROR(man.SerializeState(stateStream), PSERROR_Serialize_ScriptError, "Cannot serialize property getters"); + TS_ASSERT(man.SerializeState(stateStream)); + // (The script will die if the getter is executed) } void test_script_serialization_template()