[Core] Make object id more performant

[IXLLEGACYIXL]

PR Details

the to string in object id has an array which allocates memory, using span would remove it

Description

im not sure if that is even wanted.
problem is with the new byte[] which could be made a span but assemblyprocessor is netstandard2.0 so it doesnt detect the new string(span) so i have to use cast to readonly span

the benchmark is here

Method	Count	Mean	Error	StdDev	Gen0	Allocated
OldToString	100	3.371 us	0.0389 us	0.0478 us	2.0981	17.19 KB
NewToString	100	3.348 us	0.0662 us	0.0587 us	1.0490	8.59 KB
WithNewStringAndSpan	100	3.161 us	0.0196 us	0.0164 us	1.0490	8.59 KB
OldToString	200	7.602 us	0.2865 us	0.8448 us	4.2038	34.38 KB
NewToString	200	7.535 us	0.1491 us	0.2573 us	2.0905	17.19 KB
WithNewStringAndSpan	200	6.345 us	0.1081 us	0.1367 us	2.0981	17.19 KB
OldToString	2000	63.012 us	1.0263 us	0.9600 us	41.9922	343.75 KB
NewToString	2000	64.608 us	1.2845 us	2.6528 us	20.9961	171.88 KB
WithNewStringAndSpan	2000	61.151 us	1.1198 us	0.9926 us	20.9961	171.88 KB

so memory allocation is halfed and object id is frequently called
performance wise it doesnt do too much

would the assemblyprocessor be 2.1 then the "faster" way with new string(span) could be chosen but im not sure why the assemblyprocessor is netstandard2.0 and not netstandard2.1

Motivation and Context

halfing the allocation in a frequently called method

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My change requires a change to the documentation.
• I have added tests to cover my changes.
• All new and existing tests passed.

help

idk how that works with the assembly processor, as this file is referenced by it so i think it doesnt need a commit?
a test of it would be good, if a bigger project opens it and it works then it should work, i couldnt make projects fail with that

i had to repair in stride.core.targets the assemblyprocessor path as i cant buid else stride anymore, this is not inlcuded in that PR but should be considered that someone with normal stride can build it

[IXLLEGACYIXL]

there would be a faster option but it faces the same problem : netstandard2.0

string.Create doesnt exist in netstandard2.0

public override string ToString()
        {
            fixed (uint* hashStart = &hash1)
            {
                return string.Create(HashStringLength, (IntPtr)hashStart, (c, state) => {
            
                    var hashBytes = (byte*)state;
                    for (var i = 0; i < HashStringLength; ++i)
                    {
                        var index0 = i >> 1;
                        var b = (byte)(hashBytes[index0] >> 4);
                        c[i++] = HexDigits[b];

                        b = (byte)(hashBytes[index0] & 0x0F);
                        c[i] = HexDigits[b];
                    }
 
                });
            }
        }

Method	Count	Mean	Error	StdDev	Gen0	Allocated
OldToString	100	3.495 us	0.0693 us	0.1158 us	2.1019	17.19 KB
NewToString	100	3.072 us	0.0611 us	0.1055 us	1.0490	8.59 KB
OldToString	200	6.524 us	0.1262 us	0.1966 us	4.2038	34.38 KB
NewToString	200	5.577 us	0.0686 us	0.0608 us	2.0981	17.19 KB
OldToString	2000	61.874 us	0.7445 us	0.6964 us	41.9922	343.75 KB
NewToString	2000	57.129 us	1.0025 us	0.8887 us	20.9961	171.88 KB

[IXLLEGACYIXL]

the new toguid is around 2-4 times faster than the old one, but again its netstandard 2.0 so it cant be used

the original intention to half the allocation is achieved, the mini performance gain probably doesnt matter after all and isnt worth the trouble

in a 15 min session tostring was in the top memory allocating methods

[manio143]

im not sure why the assemblyprocessor is netstandard2.0 and not netstandard2.1

It has to be compatible with .NET Framework 4.7.2 which is what VS is targeting. And that framework version only supports netstandard2.0

[Ethereal77]

Note you can surround with #if NETSTANDARD2_0 the parts that are exclusive to the AssemblyProcessor and still keep the fast path for .NET 5+ so the runtime uses the better alternative.

[IXLLEGACYIXL]

updated help section, is ready for review and extra tests

Note you can surround with #if NETSTANDARD2_0 the parts that are exclusive to the AssemblyProcessor and still keep the fast path for .NET 5+ so the runtime uses the better alternative.

i will just go with the current version so it makes least amount of confusion
i dont think its worth the effort to maintain old + new

the main problem why i looked into it was the allocation as object id to string is often used

for referenc a version without unsafe but its slower in tostring but ALOT faster in toguid and the byte operator
but that would require net6 and other parts even net7 so it would get too complex

EDIT:
if there is interest i can make the doubled version

forgot the reference lololo https://paste.mod.gg/gifhzrqjvzfl/0
it wasnt made entirely by me, i got help with it

[IXLLEGACYIXL]

seems like assembly procsoor doestn get build, dont know why i can build it with the changes

would be nice if someone can take over the PR and help me with that ( if its not casued iwth the current situation that it cant build )

[manio143]

I'll have a look at making the 2 versions (netstandard and net6+) in this PR - thanks for starting it

[IXLLEGACYIXL]

I'll have a look at making the 2 versions (netstandard and net6+) in this PR - thanks for starting it

Thank you!

public override string ToString()
        {
            fixed (uint* hashStart = &hash1)
            {
                return string.Create(HashStringLength, (IntPtr)hashStart, (c, state) => {
            
                    var hashBytes = (byte*)state;
                    for (var i = 0; i < HashStringLength; ++i)
                    {
                        var index0 = i >> 1;
                        var b = (byte)(hashBytes[index0] >> 4);
                        c[i++] = HexDigits[b];

                        b = (byte)(hashBytes[index0] & 0x0F);
                        c[i] = HexDigits[b];
                    }
 
                });
            }
        }

this is the fastest one i was able to make ( with help )

in the linked resource its as double as slow as the original one, so dont take that one :D

	public Guid ToGuid()
	{
		return MemoryMarshal.Cast<ObjectId, Guid>(
			MemoryMarshal.CreateReadOnlySpan(ref this, 1))[0];
	}

this was 2-4 times faster but requires also netstandard2.1 or net6

this should be also faster

	public static explicit operator ObjectId(Guid guid)
	{
		return MemoryMarshal.Cast<Guid, ObjectId>(
			MemoryMarshal.CreateReadOnlySpan(ref guid, 1))[0];
	}

the guy who helped me said that simd helps here, whatever that means, but as always i only tested it with == compare, which is not much for these as i couldnt get stride working with it :D

[dotlogix]

@IXLLEGACYIXL This code can actually be simplified to

// To ObjectId
Unsafe.As<Guid, ObjectId>(ref guid)

// From ObjectId
Unsafe.As<ObjectId, Guid>(ref this)

This is a single IL instruction can't be cheaper than this 😄 no need to use spans here.
I do a lot of intrinsic stuff lately let me know if you need help with anything especially SIMD.

There is still a lot that can be improved in this struct :)

[Eideren]

I think the PR is strictly for converting them to string, otherwise we already have operators to convert back and forth between guid and objectid. See:
https://github.com/stride3d/stride/blob/master/sources/core/Stride.Core/Storage/ObjectId.cs#L68-L71
https://github.com/stride3d/stride/blob/master/sources/core/Stride.Core/Storage/ObjectId.cs#L255-L261

[IXLLEGACYIXL]

I think the PR is strictly for converting them to string, otherwise we already have operators to convert back and forth between guid and objectid. See: https://github.com/stride3d/stride/blob/master/sources/core/Stride.Core/Storage/ObjectId.cs#L68-L71 https://github.com/stride3d/stride/blob/master/sources/core/Stride.Core/Storage/ObjectId.cs#L255-L261

dotlogix gave me a suggestion on how to improve these too, i will add them
the tostring is often called

[IXLLEGACYIXL]

@IXLLEGACYIXL This code can actually be simplified to

// To ObjectId
Unsafe.As<Guid, ObjectId>(ref guid)

// From ObjectId
Unsafe.As<ObjectId, Guid>(ref this)

This is a single IL instruction can't be cheaper than this 😄 no need to use spans here. I do a lot of intrinsic stuff lately let me know if you need help with anything especially SIMD.

There is still a lot that can be improved in this struct :)

https://paste.mod.gg/gifhzrqjvzfl/0
see this link ( the tostring sucks ) that was the original plan to use but its either net7 or incompatible with netsatndard2.0 but i will add your suggestion

[IXLLEGACYIXL]

added the improvements

this shouldnt be merged as manio will make the improvements so they work depedent on the target framework

[Eideren]

Unsafe.As is pointer casting wrapped up in a method call, this won't lead to any performance improvement. If this change requires more work to be supported by the assembly processor might as well remove it I think

[IXLLEGACYIXL]

Unsafe.As is pointer casting wrapped up in a method call, this won't lead to any performance improvement. If this change requires more work to be supported by the assembly processor might as well remove it I think

the original attempt was just to hallf the memory allocation, mine currently works with both and achieves the target of halfing the allocation

the unsafe as is just a code improvement, same with the gethashcode, making it more readable

mine could be merged as is right now in terms of compatibility with the assemblyprocessor ... ofcourse i wish a test from someone else too :D

[IXLLEGACYIXL]

should manio make it with preprocessor directives or should we go with what i have now?

the current thing can be merged after a maintainer ( or someone else ) tried it, as known now assemblyprocessor recompilation leads to different results so i want to make sure i dont break anything

[manio143]

Unsafe.As is pointer casting wrapped up in a method call, this won't lead to any performance improvement. If this change requires more work to be supported by the assembly processor might as well remove it I think

This is treated specially by JIT and inlined to a pointer cast effectively. But it's supposed to be safe to use without unsafe context.

@IXLLEGACYIXL made a PR against your branch here: NexStandard#1

[IXLLEGACYIXL]

Unsafe.As is pointer casting wrapped up in a method call, this won't lead to any performance improvement. If this change requires more work to be supported by the assembly processor might as well remove it I think

This is treated specially by JIT and inlined to a pointer cast effectively. But it's supposed to be safe to use without unsafe context.

@IXLLEGACYIXL made a PR against your branch here: NexStandard#1

thank you

[IXLLEGACYIXL]

i want to run today again the tests and then it should be fine

[manio143]

I had this error locally as well, but then I rebuilt and it started working again somehow, so unclear what is causing
https://teamcity.stride3d.net/buildConfiguration/Engine_Build_BuildAssemblyProcessor/22462?hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandPull+Request+Details=true&expandBuildChangesSection=true&expandBuildProblemsSection=true

[IXLLEGACYIXL]

i cant view that

[manio143]

You can see build details as Guest - there's a link beneath login form.
#1987 fixes compilation issue.
@IXLLEGACYIXL can you push another commit (like an empty one even) to retrigger the build pipeline over your changes?