Remove unnecessary ref modifier on ParseResult<>
#96
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can I help with any hesitation to merge this? Seems rather straightforward unless I'm missing some JIT intrinsics/tricks that this is relying on and which are not obvious from the lack of IL changes. |
|
I think the general rule is that if the struct is more than int pointer size it might make a difference. On x64 this struct is borderline so if no fields are anticipated to be added, might not need the strict requirement of being passed as Not sure what the current restrictions are though? |
|
I ran the benchmarks and the changes are small, but much more on a single Parser call (SkipWhiteSpace1/10): Before After |
@lahma I'm not sure I follow your reasoning here. parlot/src/Parlot/Fluent/Parser.cs Line 7 in d0814f0 Making For example, for the following: static class C
{
public static bool M(ref ParseResult<string> r)
{
r.Set(12, 34, "Hello");
return true;
}
public static bool M(ref ParseResult<int> r)
{
r.Set(12, 34, 56);
return true;
}
}
public /* ref */ struct ParseResult<T>
{
public void Set(int start, int end, T value)
{
Start = start;
End = end;
Value = value;
}
public int Start;
public int End;
public T Value;
}The compiled native code will be identical whether the ; Core CLR 8.0.424.16909 on x86
C.M(ParseResult`1<System.String> ByRef)
L0000: mov dword ptr [ecx+4], 0xc
L0007: mov dword ptr [ecx+8], 0x22
L000e: mov eax, [0x8f83360]
L0013: mov edx, ecx
L0015: call 0x72138004
L001a: mov eax, 1
L001f: ret
C.M(ParseResult`1<Int32> ByRef)
L0000: mov dword ptr [ecx], 0xc
L0006: mov dword ptr [ecx+4], 0x22
L000d: mov dword ptr [ecx+8], 0x38
L0014: mov eax, 1
L0019: ret
ParseResult`1.Set(...)
; Open generics cannot be JIT-compiled.
; However you can use attribute SharpLab.Runtime.JitGeneric to specify argument types.
; Example: [JitGeneric(typeof(int)), JitGeneric(typeof(string))] void M<T>() { ... }.And along the same lines for x64: C.M (ParseResult`1&)
L0000 mov dword ptr [rcx+8], 0xc
L0007 mov dword ptr [rcx+0xc], 0x22
L000e mov rdx, 0x198318258f0
L0018 mov rdx, [rdx]
L001b call 0x00007fffb9acbf40
L0020 mov eax, 1
L0025 ret
C.M (ParseResult`1&)
L0000 mov dword ptr [rcx], 0xc
L0006 mov dword ptr [rcx+4], 0x22
L000d mov dword ptr [rcx+8], 0x38
L0014 mov eax, 1
L0019 ret
@sebastienros I'm seeing somewhat different results. Before this PR:
and after this PR:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes the
refmodifier onParseResult<>that seems to be unused and unnecessarily could post restrictions on usage. Its removal does not seem to have any bearing on the compiled code:The above delta was produced as follows (commit f1586c8 below was the head of
mainat time of opening this PR):