Skip to content

Commit

Permalink
C#: Ensure we only create one CSharpScript per type
Browse files Browse the repository at this point in the history 
Previously, for each scripts class instance that was created from code
rather than by the engine, we were constructing, configuring and
assigning a new CSharpScript.
This has changed now and we make sure there's only one CSharpScript
associated to each type.
  • Loading branch information
@neikeq
neikeq committed Aug 22, 2022
1 parent 92503ae commit 67db899
Show file tree
Hide file tree
Showing 9 changed files with 141 additions and 136 deletions.
50 changes: 35 additions & 15 deletions modules/mono/csharp_script.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ Error CSharpLanguage::execute_file(const String &p_path) {
return OK;
}

extern void *godotsharp_pinvoke_funcs[179];
extern void *godotsharp_pinvoke_funcs[181];
[[maybe_unused]] volatile void **do_not_strip_godotsharp_pinvoke_funcs;
#ifdef TOOLS_ENABLED
extern void *godotsharp_editor_pinvoke_funcs[30];
Expand Down Expand Up @@ -793,6 +793,20 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) {
// (while hot-reload is not yet implemented)
gdmono->initialize_load_assemblies();

{
MutexLock lock(script_instances_mutex);

for (SelfList<CSharpScript> *elem = script_list.first(); elem; elem = elem->next()) {
Ref<CSharpScript> script(elem->self());

script->exports_invalidated = true;

if (!script->get_path().is_empty()) {
script->reload(p_soft_reload);
}
}
}

#if 0
// There is no soft reloading with Mono. It's always hard reloading.

Expand Down Expand Up @@ -1010,7 +1024,7 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) {

GDMonoClass *native = GDMonoUtils::get_class_native_base(script_class);

CSharpScript::initialize_for_managed_type(script, script_class, native);
CSharpScript::reload_registered_script(script, script_class, native);
}

StringName native_name = NATIVE_GDMONOCLASS_NAME(script->native);
Expand Down Expand Up @@ -1563,9 +1577,13 @@ void CSharpLanguage::tie_native_managed_to_unmanaged(GCHandleIntPtr p_gchandle_i
CSharpLanguage::set_instance_binding(p_unmanaged, data);
}

void CSharpLanguage::tie_user_managed_to_unmanaged(GCHandleIntPtr p_gchandle_intptr, Object *p_unmanaged, CSharpScript *p_script, bool p_ref_counted) {
void CSharpLanguage::tie_user_managed_to_unmanaged(GCHandleIntPtr p_gchandle_intptr, Object *p_unmanaged, Ref<CSharpScript> *p_script, bool p_ref_counted) {
// This method should not fail

Ref<CSharpScript> script = *p_script;
// We take care of destructing this reference here, so the managed code won't need to do another P/Invoke call
p_script->~Ref();

CRASH_COND(!p_unmanaged);

// All mono objects created from the managed world (e.g.: 'new Player()')
Expand All @@ -1578,12 +1596,8 @@ void CSharpLanguage::tie_user_managed_to_unmanaged(GCHandleIntPtr p_gchandle_int
MonoGCHandleData gchandle = MonoGCHandleData(p_gchandle_intptr,
p_ref_counted ? gdmono::GCHandleType::WEAK_HANDLE : gdmono::GCHandleType::STRONG_HANDLE);

Ref<CSharpScript> script = p_script;

CRASH_COND(script.is_null());

CSharpScript::initialize_for_managed_type(script);

CSharpInstance *csharp_instance = CSharpInstance::create_for_managed_type(p_unmanaged, script.ptr(), gchandle);

p_unmanaged->set_script_and_instance(script, csharp_instance);
Expand Down Expand Up @@ -2345,12 +2359,17 @@ void CSharpScript::_bind_methods() {
ClassDB::bind_vararg_method(METHOD_FLAGS_DEFAULT, "new", &CSharpScript::_new, MethodInfo("new"));
}

void CSharpScript::initialize_for_managed_type(Ref<CSharpScript> p_script) {
void CSharpScript::reload_registered_script(Ref<CSharpScript> p_script) {
// IMPORTANT:
// This method must be called only after the CSharpScript and its associated type
// have been added to the script bridge map in the ScriptManagerBridge C# class.
// Other than that, it's the same as `CSharpScript::reload`.

// This method should not fail, only assertions allowed
// This method should not fail, only assertions allowed.

// Unlike `reload`, we print an error rather than silently returning,
// as we can assert this won't be called a second time until invalidated.
ERR_FAIL_COND(!p_script->reload_invalidated);

p_script->valid = true;
p_script->reload_invalidated = false;
Expand Down Expand Up @@ -2607,9 +2626,6 @@ Error CSharpScript::reload(bool p_keep_state) {

String script_path = get_path();

// In case it was already added by a previous reload
GDMonoCache::managed_callbacks.ScriptManagerBridge_RemoveScriptBridge(this);

valid = GDMonoCache::managed_callbacks.ScriptManagerBridge_AddScriptBridge(this, &script_path);

if (valid) {
Expand Down Expand Up @@ -2817,9 +2833,13 @@ Ref<Resource> ResourceFormatLoaderCSharpScript::load(const String &p_path, const

// TODO ignore anything inside bin/ and obj/ in tools builds?

CSharpScript *script = memnew(CSharpScript);
Ref<CSharpScript> script;

Ref<CSharpScript> scriptres(script);
if (GDMonoCache::godot_api_cache_updated) {
GDMonoCache::managed_callbacks.ScriptManagerBridge_GetOrCreateScriptBridgeForPath(&p_path, &script);
} else {
script = Ref<CSharpScript>(memnew(CSharpScript));
}

#if defined(DEBUG_ENABLED) || defined(TOOLS_ENABLED)
Error err = script->load_source_code(p_path);
Expand All @@ -2834,7 +2854,7 @@ Ref<Resource> ResourceFormatLoaderCSharpScript::load(const String &p_path, const
*r_error = OK;
}

return scriptres;
return script;
}

void ResourceFormatLoaderCSharpScript::get_recognized_extensions(List<String> *p_extensions) const {
Expand Down
5 changes: 3 additions & 2 deletions modules/mono/csharp_script.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ class CSharpScript : public Script {

// Do not use unless you know what you are doing
static void update_script_class_info(Ref<CSharpScript> p_script);
static void initialize_for_managed_type(Ref<CSharpScript> p_script);

protected:
static void _bind_methods();
Expand All @@ -144,6 +143,8 @@ class CSharpScript : public Script {
void _get_property_list(List<PropertyInfo> *p_properties) const;

public:
static void reload_registered_script(Ref<CSharpScript> p_script);

bool can_instantiate() const override;
StringName get_instance_base_type() const override;
ScriptInstance *instance_create(Object *p_this) override;
Expand Down Expand Up @@ -460,7 +461,7 @@ class CSharpLanguage : public ScriptLanguage {
bool setup_csharp_script_binding(CSharpScriptBinding &r_script_binding, Object *p_object);

static void tie_native_managed_to_unmanaged(GCHandleIntPtr p_gchandle_intptr, Object *p_unmanaged, const StringName *p_native_name, bool p_ref_counted);
static void tie_user_managed_to_unmanaged(GCHandleIntPtr p_gchandle_intptr, Object *p_unmanaged, CSharpScript *p_script, bool p_ref_counted);
static void tie_user_managed_to_unmanaged(GCHandleIntPtr p_gchandle_intptr, Object *p_unmanaged, Ref<CSharpScript> *p_script, bool p_ref_counted);
static void tie_managed_to_unmanaged_with_pre_setup(GCHandleIntPtr p_gchandle_intptr, Object *p_unmanaged);

#warning TODO
Expand Down
2 changes: 2 additions & 0 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ManagedCallbacks.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public unsafe struct ManagedCallbacks
public delegate* unmanaged<IntPtr, godot_string*, godot_bool> ScriptManagerBridge_HasScriptSignal;
public delegate* unmanaged<IntPtr, IntPtr, godot_bool> ScriptManagerBridge_ScriptIsOrInherits;
public delegate* unmanaged<IntPtr, godot_string*, godot_bool> ScriptManagerBridge_AddScriptBridge;
public delegate* unmanaged<godot_string*, godot_ref*, void> ScriptManagerBridge_GetOrCreateScriptBridgeForPath;
public delegate* unmanaged<IntPtr, void> ScriptManagerBridge_RemoveScriptBridge;
public delegate* unmanaged<IntPtr, godot_bool*, godot_dictionary*, void> ScriptManagerBridge_UpdateScriptClassInfo;
public delegate* unmanaged<IntPtr, IntPtr*, godot_bool, godot_bool> ScriptManagerBridge_SwapGCHandleForType;
Expand Down Expand Up @@ -56,6 +57,7 @@ public static ManagedCallbacks Create()
ScriptManagerBridge_HasScriptSignal = &ScriptManagerBridge.HasScriptSignal,
ScriptManagerBridge_ScriptIsOrInherits = &ScriptManagerBridge.ScriptIsOrInherits,
ScriptManagerBridge_AddScriptBridge = &ScriptManagerBridge.AddScriptBridge,
ScriptManagerBridge_GetOrCreateScriptBridgeForPath = &ScriptManagerBridge.GetOrCreateScriptBridgeForPath,
ScriptManagerBridge_RemoveScriptBridge = &ScriptManagerBridge.RemoveScriptBridge,
ScriptManagerBridge_UpdateScriptClassInfo = &ScriptManagerBridge.UpdateScriptClassInfo,
ScriptManagerBridge_SwapGCHandleForType = &ScriptManagerBridge.SwapGCHandleForType,
Expand Down
94 changes: 62 additions & 32 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,11 @@ namespace Godot.Bridge
{
public static class ScriptManagerBridge
{
private static System.Collections.Generic.Dictionary<string, ScriptLookupInfo> _scriptLookupMap = new();
private static System.Collections.Generic.Dictionary<IntPtr, Type> _scriptBridgeMap = new();
private static System.Collections.Generic.Dictionary<string, Type> _pathScriptMap = new();

private struct ScriptLookupInfo
{
public string ClassNamespace { get; private set; }
public string ClassName { get; private set; }
public Type ScriptType { get; private set; }

public ScriptLookupInfo(string classNamespace, string className, Type scriptType)
{
ClassNamespace = classNamespace;
ClassName = className;
ScriptType = scriptType;
}
};
private static readonly object ScriptBridgeLock = new();
private static System.Collections.Generic.Dictionary<IntPtr, Type> _scriptTypeMap = new();
private static System.Collections.Generic.Dictionary<Type, IntPtr> _typeScriptMap = new();

[UnmanagedCallersOnly]
internal static void FrameCallback()
Expand Down Expand Up @@ -80,7 +69,7 @@ internal static void FrameCallback()
try
{
// Performance is not critical here as this will be replaced with source generators.
Type scriptType = _scriptBridgeMap[scriptPtr];
Type scriptType = _scriptTypeMap[scriptPtr];
var obj = (Object)FormatterServices.GetUninitializedObject(scriptType);

var ctor = scriptType
Expand Down Expand Up @@ -133,7 +122,7 @@ internal static unsafe void GetScriptNativeName(IntPtr scriptPtr, godot_string_n
try
{
// Performance is not critical here as this will be replaced with source generators.
if (!_scriptBridgeMap.TryGetValue(scriptPtr, out var scriptType))
if (!_scriptTypeMap.TryGetValue(scriptPtr, out var scriptType))
{
*outRes = default;
return;
Expand Down Expand Up @@ -225,7 +214,7 @@ static void LookupScriptForClass(Type type)
if (scriptPathAttr == null)
return;

_scriptLookupMap[scriptPathAttr.Path] = new ScriptLookupInfo(type.Namespace, type.Name, type);
_pathScriptMap[scriptPathAttr.Path] = type;
}

var assemblyHasScriptsAttr = assembly.GetCustomAttributes(inherit: false)
Expand Down Expand Up @@ -304,7 +293,7 @@ internal static unsafe void GetScriptSignalList(IntPtr scriptPtr, godot_dictiona
// Performance is not critical here as this will be replaced with source generators.
using var signals = new Dictionary();

Type top = _scriptBridgeMap[scriptPtr];
Type top = _scriptTypeMap[scriptPtr];
Type native = Object.InternalGetClassNativeBase(top);

while (top != null && top != native)
Expand Down Expand Up @@ -400,7 +389,7 @@ internal static unsafe godot_bool HasScriptSignal(IntPtr scriptPtr, godot_string

string signalNameStr = Marshaling.ConvertStringToManaged(*signalName);

Type top = _scriptBridgeMap[scriptPtr];
Type top = _scriptTypeMap[scriptPtr];
Type native = Object.InternalGetClassNativeBase(top);

while (top != null && top != native)
Expand Down Expand Up @@ -446,10 +435,10 @@ internal static godot_bool ScriptIsOrInherits(IntPtr scriptPtr, IntPtr scriptPtr
{
try
{
if (!_scriptBridgeMap.TryGetValue(scriptPtr, out var scriptType))
if (!_scriptTypeMap.TryGetValue(scriptPtr, out var scriptType))
return false.ToGodotBool();

if (!_scriptBridgeMap.TryGetValue(scriptPtrMaybeBase, out var maybeBaseType))
if (!_scriptTypeMap.TryGetValue(scriptPtrMaybeBase, out var maybeBaseType))
return false.ToGodotBool();

return (scriptType == maybeBaseType || maybeBaseType.IsAssignableFrom(scriptType)).ToGodotBool();
Expand All @@ -466,12 +455,19 @@ internal static unsafe godot_bool AddScriptBridge(IntPtr scriptPtr, godot_string
{
try
{
string scriptPathStr = Marshaling.ConvertStringToManaged(*scriptPath);
lock (ScriptBridgeLock)
{
if (!_scriptTypeMap.ContainsKey(scriptPtr))
{
string scriptPathStr = Marshaling.ConvertStringToManaged(*scriptPath);

if (!_scriptLookupMap.TryGetValue(scriptPathStr, out var lookupInfo))
return false.ToGodotBool();
if (!_pathScriptMap.TryGetValue(scriptPathStr, out Type scriptType))
return false.ToGodotBool();

_scriptBridgeMap.Add(scriptPtr, lookupInfo.ScriptType);
_scriptTypeMap.Add(scriptPtr, scriptType);
_typeScriptMap.Add(scriptType, scriptPtr);
}
}

return true.ToGodotBool();
}
Expand All @@ -482,15 +478,49 @@ internal static unsafe godot_bool AddScriptBridge(IntPtr scriptPtr, godot_string
}
}

internal static void AddScriptBridgeWithType(IntPtr scriptPtr, Type scriptType)
=> _scriptBridgeMap.Add(scriptPtr, scriptType);
[UnmanagedCallersOnly]
internal static unsafe void GetOrCreateScriptBridgeForPath(godot_string* scriptPath, godot_ref* outScript)
{
string scriptPathStr = Marshaling.ConvertStringToManaged(*scriptPath);

if (!_pathScriptMap.TryGetValue(scriptPathStr, out Type scriptType))
{
NativeFuncs.godotsharp_internal_new_csharp_script(outScript);
return;
}

GetOrCreateScriptBridgeForType(scriptType, outScript);
}

internal static unsafe void GetOrCreateScriptBridgeForType(Type scriptType, godot_ref* outScript)
{
lock (ScriptBridgeLock)
{
if (_typeScriptMap.TryGetValue(scriptType, out IntPtr scriptPtr))
{
NativeFuncs.godotsharp_ref_new_from_ref_counted_ptr(out *outScript, scriptPtr);
return;
}

NativeFuncs.godotsharp_internal_new_csharp_script(outScript);
scriptPtr = outScript->Reference;

_scriptTypeMap.Add(scriptPtr, scriptType);
_typeScriptMap.Add(scriptType, scriptPtr);

NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
}
}

[UnmanagedCallersOnly]
internal static void RemoveScriptBridge(IntPtr scriptPtr)
{
try
{
_scriptBridgeMap.Remove(scriptPtr);
lock (ScriptBridgeLock)
{
_ = _scriptTypeMap.Remove(scriptPtr);
}
}
catch (Exception e)
{
Expand All @@ -505,7 +535,7 @@ internal static void RemoveScriptBridge(IntPtr scriptPtr)
try
{
// Performance is not critical here as this will be replaced with source generators.
var scriptType = _scriptBridgeMap[scriptPtr];
var scriptType = _scriptTypeMap[scriptPtr];

*outTool = scriptType.GetCustomAttributes(inherit: false)
.OfType<ToolAttribute>()
Expand Down Expand Up @@ -630,7 +660,7 @@ public void Dispose()
{
try
{
Type scriptType = _scriptBridgeMap[scriptPtr];
Type scriptType = _scriptTypeMap[scriptPtr];
GetPropertyInfoListForType(scriptType, scriptPtr, addPropInfoFunc);
}
catch (Exception e)
Expand Down Expand Up @@ -752,7 +782,7 @@ public void Dispose()
{
try
{
Type top = _scriptBridgeMap[scriptPtr];
Type top = _scriptTypeMap[scriptPtr];
Type native = Object.InternalGetClassNativeBase(top);

while (top != null && top != native)
Expand Down
18 changes: 11 additions & 7 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropUtils.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,17 @@ public static Object UnmanagedGetManaged(IntPtr unmanaged)
}
else
{
IntPtr scriptPtr = NativeFuncs.godotsharp_internal_new_csharp_script();

ScriptManagerBridge.AddScriptBridgeWithType(scriptPtr, type);

// IMPORTANT: This must be called after AddScriptWithTypeBridge
NativeFuncs.godotsharp_internal_tie_user_managed_to_unmanaged(
GCHandle.ToIntPtr(gcHandle), unmanaged, scriptPtr, refCounted.ToGodotBool());
unsafe
{
// We don't dispose `script` ourselves here.
// `tie_user_managed_to_unmanaged` does it for us to avoid another P/Invoke call.
godot_ref script;
ScriptManagerBridge.GetOrCreateScriptBridgeForType(type, &script);

// IMPORTANT: This must be called after GetOrCreateScriptBridgeForType
NativeFuncs.godotsharp_internal_tie_user_managed_to_unmanaged(
GCHandle.ToIntPtr(gcHandle), unmanaged, &script, refCounted.ToGodotBool());
}
}
}

Expand Down

0 comments on commit 67db899

Please sign in to comment.