Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup structure #1734

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

cleanup structure #1734

wants to merge 3 commits into from

Conversation

neuecc
Copy link
Member

@neuecc neuecc commented Jan 12, 2024

@AArnott @pCYSl5EDgo

This is a request for a significant change in the project structure.
Currently, all the source code placed on the Unity side is to be moved to the .NET side, with Unity referencing a dll.
This change will make it easier to write as a .NET library, and also allows us to adopt C# 12.
There is no need to be conscious of Unity in regular code writing.

On the Unity side, it will reference a NuGet library built as a DLL as its core,
and use a plugin method to separately reference extensions for Unity through Unity UPM.

MessagePack for C# is widely referenced as a foundational library, but
since there is no compatibility between the Unity version and the NuGet version of the library,
NuGet libraries that depend on MessagePack for C# in Unity have become unreferenceable.
Unity is now different from the past, being .NET Standard 2.1,
and excellent package managers like NuGetForUnity have also emerged.

I recently released a new library called R3, and there, I tried the same structure as this PR.
https://github.com/Cysharp/R3/#unity

I believe adopting this PR is important for the efficiency of future development and the progress in Unity.

However, since this would break compatibility with references on the Unity side,
how about timing the release to coincide with the Source Generatorization when mpc is no longer needed?

@AArnott AArnott added this to the v2.6 milestone Jan 12, 2024
@AArnott
Copy link
Collaborator

AArnott commented Jan 12, 2024

That all sounds awesome. I'll review today or this weekend. I expect it'll cause massive merge conflicts with my work on #1691, so I'll suspend that work for now to not compound the problem.

@AArnott
Copy link
Collaborator

AArnott commented Jan 12, 2024

Should we bump the major version number given that it'll be a breaking change for unity folks, as well as users of the analyzer?

@AArnott
Copy link
Collaborator

AArnott commented Jan 12, 2024

How does this impact any hesitancy to take on other nuget dependencies? In the past, I think dependencies on other assemblies were taken only with huge justification because it complicated consumption for unity users.

@pCYSl5EDgo
Copy link
Contributor

pCYSl5EDgo commented Jan 13, 2024

Should we bump the major version number given that it'll be a breaking change for unity folks, as well as users of the analyzer?

Major version 3 is appropriate because the mpc deletion is big breaking change.

Additional Info

Unity 2021.3 is using Roslyn v3 and it dies in 2024 April.
C#11 is available since Roslyn v4.4 (Visual Studio v17.4).

@neuecc
Copy link
Member Author

neuecc commented Jan 13, 2024

In the past, I think dependencies on other assemblies were taken only with huge justification because it complicated consumption for unity users.

Disable Assembly Version Validation solves it.
image

II also think 3 is a good choice. Converting to Source Generator would be a significant change for all users.


This draft is rough and there are several tasks remaining.

  • Need to remove code like if UNITY or IF IL2CPP from the Core
  • Especially for Unity Android, there was code tailored for binary Write/Read, and we need to decide what to do with that
  • There was an issue with sharing test code because signed InternalVisibleTo couldn't be referenced on the Unity side. (It was only for Sequence, so I temporarily pasted the Sequence itself to remove the compile error.)

Since NuGetForUnity also automatically reflects Source Generators, creating SourceGenerators for both 3.x and 4.x and including the dlls should automatically segregate them, which would be good. Let's not create a SourceGenerator package specifically for Unity.

By the way, Unity supports Incremental Generators depending on the version, and currently, it seems that versions 4.1.0(Unity 2022.2) and 4.3.0(latest) are available.

@AArnott
Copy link
Collaborator

AArnott commented Jan 14, 2024

All this sounds exciting. Do you mind holding off on this substantial refactoring until my source generator/analyzer work to switch to attributes is done?

@pCYSl5EDgo
Copy link
Contributor

pCYSl5EDgo commented Jan 14, 2024

Dropping netstandard2.0 and update to netstandard2.1 in MessagePack csprojs (except Source Generator) will be permitted?
Or keep it because SignalR depends on netstandard2.0?

@AArnott
Copy link
Collaborator

AArnott commented Jan 14, 2024

No, we can't drop netstandard2.0. I believe what neuecc is saying is because unity supports netstandard2.1, we don't need a special source-based build for it any more.

@neuecc
Copy link
Member Author

neuecc commented Jan 15, 2024

Do you mind holding off on this substantial refactoring until my source generator/analyzer work to switch to attributes is done?

OK.

@pCYSl5EDgo
Copy link
Contributor

Especially for Unity Android, there was code tailored for binary Write/Read, and we need to decide what to do with that

I've installed Unity2022.3.18 and built IL2CPP in order to assess the cost of runtime Android armv7 detection.
All of the latter C++ codes are generated with windows x64 master build settings.

Results

  • BitConverter.IsLittleEndian is compiled to be an inlined function returning constant boolean.
  • sizeof(nint) == 4 is compiled to a constant-like expression.
  • RuntimeInformation.ProcessArchitecture == Architecture.Arm is compiled to the code which accesses static field.

Considering the compile-time optimization of C++, the combination of BitConverter.IsLittleEndian and sizeof(nint) == 4 is the best method. Even if this method cannot distinguish x86-windows and armv7-android, it has the best runtime efficiency.

Methods

Test C# code
using System;
using System.Collections;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using UnityEngine;

public unsafe class TestForConst : MonoBehaviour
{
    void Update()
    {
        Vector3 velocity;
        if (BitConverter.IsLittleEndian)
        {
            velocity = new(0, 1, 0);
        }
        else if (sizeof(nint) == 4)
        {
            velocity = new(0, -1, 0);
        }
        else
        {
            velocity = new();
        }

        velocity += GenericVector<int>.Get();
        
        transform.position += velocity * Time.deltaTime;
    }
}

internal struct GenericVector<T>
{
    public static Vector3 Get()
    {
        if (typeof(T) == typeof(int))
        {
            if (RuntimeInformation.ProcessArchitecture == Architecture.Arm)
            {
                return new(1, 0, 1);
            }

            return new(1, 0, 0);
        }

        return new(-1, 0, 0);
    }
}
Generated C++ code for Update method
IL2CPP_EXTERN_C IL2CPP_METHOD_ATTR void TestForConst_Update_mAECC56A194C2E9D162EAFFB3B9F23B0B4EBCC9F7 (TestForConst_tDA600D04A981E1B53CD873D2147332DB5AA8480E* __this, const RuntimeMethod* method) 
{
	static bool s_Il2CppMethodInitialized;
	if (!s_Il2CppMethodInitialized)
	{
		il2cpp_codegen_initialize_runtime_metadata((uintptr_t*)&GenericVector_1_Get_m49BFA61A2FA9A6589F796D73B626ED5C0562B4A7_RuntimeMethod_var);
		s_Il2CppMethodInitialized = true;
	}
	Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 V_0;
	memset((&V_0), 0, sizeof(V_0));
	{
		if (!il2cpp_codegen_is_little_endian())
		{
			goto IL_001f;
		}
	}
	{
		Vector3__ctor_m376936E6B999EF1ECBE57D990A386303E2283DE0_inline((&V_0), (0.0f), (1.0f), (0.0f), NULL);
		goto IL_0048;
	}

IL_001f:
	{
		uint32_t L_0 = sizeof(intptr_t);
		if ((!(((uint32_t)L_0) == ((uint32_t)4))))
		{
			goto IL_0040;
		}
	}
	{
		Vector3__ctor_m376936E6B999EF1ECBE57D990A386303E2283DE0_inline((&V_0), (0.0f), (-1.0f), (0.0f), NULL);
		goto IL_0048;
	}

IL_0040:
	{
		il2cpp_codegen_initobj((&V_0), sizeof(Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2));
	}

IL_0048:
	{
		Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 L_1 = V_0;
		Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 L_2;
		L_2 = GenericVector_1_Get_m49BFA61A2FA9A6589F796D73B626ED5C0562B4A7(GenericVector_1_Get_m49BFA61A2FA9A6589F796D73B626ED5C0562B4A7_RuntimeMethod_var);
		Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 L_3;
		L_3 = Vector3_op_Addition_m78C0EC70CB66E8DCAC225743D82B268DAEE92067_inline(L_1, L_2, NULL);
		V_0 = L_3;
		Transform_tB27202C6F4E36D225EE28A13E4D662BF99785DB1* L_4;
		L_4 = Component_get_transform_m2919A1D81931E6932C7F06D4C2F0AB8DDA9A5371(__this, NULL);
		Transform_tB27202C6F4E36D225EE28A13E4D662BF99785DB1* L_5 = L_4;
		NullCheck(L_5);
		Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 L_6;
		L_6 = Transform_get_position_m69CD5FA214FDAE7BB701552943674846C220FDE1(L_5, NULL);
		Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 L_7 = V_0;
		float L_8;
		L_8 = Time_get_deltaTime_mC3195000401F0FD167DD2F948FD2BC58330D0865(NULL);
		Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 L_9;
		L_9 = Vector3_op_Multiply_m87BA7C578F96C8E49BB07088DAAC4649F83B0353_inline(L_7, L_8, NULL);
		Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 L_10;
		L_10 = Vector3_op_Addition_m78C0EC70CB66E8DCAC225743D82B268DAEE92067_inline(L_6, L_9, NULL);
		NullCheck(L_5);
		Transform_set_position_mA1A817124BB41B685043DED2A9BA48CDF37C4156(L_5, L_10, NULL);
		return;
	}
}
il2cpp_codegen_is_little_endian is well optimized.
inline bool il2cpp_codegen_is_little_endian()
{
#if IL2CPP_BYTE_ORDER == IL2CPP_LITTLE_ENDIAN
    return true;
#else
    return false;
#endif
}
`typeof(T) == typeof(int)` is bad.
IL2CPP_EXTERN_C IL2CPP_METHOD_ATTR Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 GenericVector_1_Get_m49BFA61A2FA9A6589F796D73B626ED5C0562B4A7_gshared (const RuntimeMethod* method) 
{
	static bool s_Il2CppMethodInitialized;
	if (!s_Il2CppMethodInitialized)
	{
		il2cpp_codegen_initialize_runtime_metadata((uintptr_t*)&Int32_t680FF22E76F6EFAD4375103CBBFFA0421349384C_0_0_0_var);
		il2cpp_codegen_initialize_runtime_metadata((uintptr_t*)&RuntimeInformation_tB2DFA85FB9251AE3A3112904C9DF06C30D1D3EAF_il2cpp_TypeInfo_var);
		il2cpp_codegen_initialize_runtime_metadata((uintptr_t*)&Type_t_il2cpp_TypeInfo_var);
		s_Il2CppMethodInitialized = true;
	}
	{
		RuntimeTypeHandle_t332A452B8B6179E4469B69525D0FE82A88030F7B L_0 = { reinterpret_cast<intptr_t> (il2cpp_rgctx_type(InitializedTypeInfo(method->klass)->rgctx_data, 0)) };
		il2cpp_codegen_runtime_class_init_inline(Type_t_il2cpp_TypeInfo_var);
		Type_t* L_1;
		L_1 = Type_GetTypeFromHandle_m6062B81682F79A4D6DF2640692EE6D9987858C57(L_0, NULL);
		RuntimeTypeHandle_t332A452B8B6179E4469B69525D0FE82A88030F7B L_2 = { reinterpret_cast<intptr_t> (Int32_t680FF22E76F6EFAD4375103CBBFFA0421349384C_0_0_0_var) };
		Type_t* L_3;
		L_3 = Type_GetTypeFromHandle_m6062B81682F79A4D6DF2640692EE6D9987858C57(L_2, NULL);
		bool L_4;
		L_4 = Type_op_Equality_m99930A0E44E420A685FABA60E60BA1CC5FA0EBDC(L_1, L_3, NULL);
		if (!L_4)
		{
			goto IL_004d;
		}
	}
	{
		il2cpp_codegen_runtime_class_init_inline(RuntimeInformation_tB2DFA85FB9251AE3A3112904C9DF06C30D1D3EAF_il2cpp_TypeInfo_var);
		int32_t L_5;
		L_5 = RuntimeInformation_get_ProcessArchitecture_mB2DAF77FAF4F8F97AE4045FA7DD140D60D8BF3F3_inline(NULL);
		if ((!(((uint32_t)L_5) == ((uint32_t)2))))
		{
			goto IL_0038;
		}
	}
	{
		Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 L_6;
		memset((&L_6), 0, sizeof(L_6));
		Vector3__ctor_m376936E6B999EF1ECBE57D990A386303E2283DE0_inline((&L_6), (1.0f), (0.0f), (1.0f), NULL);
		return L_6;
	}

IL_0038:
	{
		Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 L_7;
		memset((&L_7), 0, sizeof(L_7));
		Vector3__ctor_m376936E6B999EF1ECBE57D990A386303E2283DE0_inline((&L_7), (1.0f), (0.0f), (0.0f), NULL);
		return L_7;
	}

IL_004d:
	{
		Vector3_t24C512C7B96BBABAD472002D0BA2BDA40A5A80B2 L_8;
		memset((&L_8), 0, sizeof(L_8));
		Vector3__ctor_m376936E6B999EF1ECBE57D990A386303E2283DE0_inline((&L_8), (-1.0f), (0.0f), (0.0f), NULL);
		return L_8;
	}
}

@pCYSl5EDgo
Copy link
Contributor

pCYSl5EDgo commented Jan 30, 2024

I've tested Unsafe.ReadUnaligned and found that Unity2022.3 treats Unsafe.ReadUnaligned as special.
It seems that we no longer need to take care of Android armv7 problem?

template<typename T>
inline T il2cpp_unsafe_read_unaligned(void* location)
{
    T result;
#if IL2CPP_TARGET_ARMV7 || IL2CPP_TARGET_JAVASCRIPT
    memcpy(&result, location, sizeof(T));
#else
    result = *((T*)location);
#endif
    return result;
}

template<typename T>
inline void il2cpp_unsafe_write_unaligned(void* location, T value)
{
#if IL2CPP_TARGET_ARMV7 || IL2CPP_TARGET_JAVASCRIPT
    memcpy(location, &value, sizeof(T));
#else
    *((T*)location) = value;
#endif
}

template<typename T, typename TOffset>
inline T* il2cpp_unsafe_add(void* source, TOffset offset)
{
    return reinterpret_cast<T*>(source) + offset;
}

template<typename T, typename TOffset>
inline T* il2cpp_unsafe_add_byte_offset(void* source, TOffset offset)
{
    return reinterpret_cast<T*>(reinterpret_cast<uint8_t*>(source) + offset);
}

By the way, Unity does not generate intrinsics for MemoryMarshal.GetReference and Unsafe.AddByteOffset unfortunately.
There seems to be il2cpp_unsafe_add_byte_offset but not used even if Unsafe.AddByteOffset is used!
In contrast, il2cpp_unsafe_add is used when Unsafe.Add is used.
Very surprising.

@AArnott
Copy link
Collaborator

AArnott commented Apr 30, 2024

I think it's time to focus on this PR. Since I introduced all the merge conflicts, I'll try to find time to freshen it up.

@AArnott
Copy link
Collaborator

AArnott commented Apr 30, 2024

Ok, it's freshened up, ready for someone with more Unity experience to patch up the rest. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants