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

Avoid using MemoryMarshal.Cast<byte, _> on Android #932

Merged
merged 1 commit into from
Jun 6, 2020

Conversation

AArnott
Copy link
Collaborator

@AArnott AArnott commented May 17, 2020

This centralizes all uses of this API to a SafeBitConverter class which does the efficient thing except on Android, where we fall back to a safe pattern.

Fixes #931
See also #748, which was a similar fix but only to two call sites.

@AArnott AArnott added this to the v2.1 milestone May 17, 2020
@AArnott AArnott requested a review from neuecc May 17, 2020 13:39
@AArnott AArnott self-assigned this May 17, 2020
@AArnott
Copy link
Collaborator Author

AArnott commented May 17, 2020

@guo-sun Can you please test that this works for you?
@neuecc I haven't tested this on Android at all so please look meticulously during the review or if you have the means to test it, please do.

@gsuuon
Copy link
Contributor

gsuuon commented May 18, 2020

Yup this fixes the crash!

I had to add some casts to get this to compile, e.g here:

return (ushort)(value[0] | (value[1] << 8))

MemoryMarshal.Cast does some checks on TTo, TFrom which slows it down a bit - does it make sense to just use the bitwise code? I couldn't see a perf difference when I tried running the benchmarks in sandbox.

public static ReadOnlySpan<TTo> Cast<TFrom, TTo>(ReadOnlySpan<TFrom> span) where TFrom : struct where TTo : struct
{
	if (SpanHelpers.IsReferenceOrContainsReferences<TFrom>())
	{
		ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(TFrom));
	}
	if (SpanHelpers.IsReferenceOrContainsReferences<TTo>())
	{
		ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(TTo));
	}
	checked
	{
		int length = (int)unchecked(checked(unchecked((long)span.Length) * unchecked((long)Unsafe.SizeOf<TFrom>())) / Unsafe.SizeOf<TTo>());
		return new ReadOnlySpan<TTo>(Unsafe.As<Pinnable<TTo>>(span.Pinnable), span.ByteOffset, length);
	}
}

This centralizes *all* uses of this API to a `SafeBitConverter` class which does the efficient thing except on Android, where we fall back to a safe pattern.

Fixes MessagePack-CSharp#931
See also MessagePack-CSharp#748, which was a similar fix but only to two call sites.
@AArnott
Copy link
Collaborator Author

AArnott commented May 18, 2020

Thanks for testing, @guo-sun. I've added the missing casts.
I suspect asking the CPU to read a 32-bit or 64-bit value is faster than asking it to read 8 8-bit values from contiguous memory. But if MemoryMarshal.Cast ever showed up on the perf trace due to validations, we could switch to Unsafe.As<T> which is what it uses internally after passing the checks.

@AArnott AArnott merged commit c9fc35e into MessagePack-CSharp:master Jun 6, 2020
@AArnott AArnott deleted the fix931 branch August 3, 2020 14:46
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.

il2cpp - MemoryMarshal.Cast can cause SIGBUS on ARMv7
2 participants