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.

Sign up for GitHub

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

Jump to bottom

Make ObjCRuntime.Runtime.Arch a constant for the AOT compiler #5518

Closed
rolfbjarne opened this issue Jan 30, 2019 · 3 comments · Fixed by #14076
Closed

Make ObjCRuntime.Runtime.Arch a constant for the AOT compiler #5518

rolfbjarne opened this issue Jan 30, 2019 · 3 comments · Fixed by #14076
Assignees
@rolfbjarne
Labels
breaking-change If an issue or a pull request represents a breaking change enhancement The issue or pull request is an enhancement iOS Issues affecting Xamarin.iOS
Milestone
XAMCORE_4_0

Comments

@rolfbjarne
Copy link
Member

rolfbjarne commented Jan 30, 2019 •
edited
Loading

Suggestion:

  • Tell the AOT compiler that the ObjCRuntime.Runtime.Arch field is a constant value (since for the AOT compiler it will always be ARCH.Device = 0), which will allow the AOT compiler to skip a lot of dead code (all the simulator-only code can be optimized away).

Requirements:

POC: rolfbjarne@84f35cf

POC result: The don't link app compiled for iOS/arm64 is ~175kb smaller.
@rolfbjarne rolfbjarne added enhancement The issue or pull request is an enhancement iOS Issues affecting Xamarin.iOS breaking-change If an issue or a pull request represents a breaking change labels Jan 30, 2019
@rolfbjarne rolfbjarne added this to the XAMCORE_4_0 milestone Jan 30, 2019
@chamons chamons mentioned this issue Jan 30, 2019
Closed
44 tasks
@spouliot
Copy link
Contributor

Where are the savings coming from ?

The linker optimize that (at least in bindings) before it reach the AOT compiler (so the field should not be present). If the saving comes from elsewhere then we could expand on this (linker) optimization.

Or maybe we can shadow the current field with a 2nd (read-only) one ? and make sure the platform assemblies (and generated code?) use this field.

Of course existing (3rd party) bindings won't make use of the new field and won't have this optimizations. Recompiling them would make them incompatible with earlier versions too :|

Now It could be internal since our platform bindings already have special case / awareness of their unique situation. OTOH that would mean re-compiling 3rd party bindings won't help.

@rolfbjarne
Copy link
Member Author

The linker optimize that (at least in bindings) before it reach the AOT compiler

This is dont link 😄 That said, the linker will only optimize this for methods with [BindingImpl (Optimizable)], the AOT optimizes everywhere.

Or maybe we can shadow the current field with a 2nd (read-only) one ? and make sure the platform assemblies (and generated code?) use this field.

That's an intriguing idea.

@spouliot
Copy link
Contributor

Ok, still make sense for Don't link wrt the interpreter, at least until we revisit all the linker optimizations :)

rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Feb 4, 2022
@rolfbjarne
[runtime] Make Runtime.Arch a readonly field in .NET. Fixes xamarin#5518
6af81c9 
.

* Make Runtime.Arch a readonly field.
* Tell the AOT compiler Runtime.Arch is a constant value.
* Tell the linker to stub out the method we use to fetch the current
  architecture from native code (it turned out a bit complicated to set the
  Arch field when it's readonly - the solution I came up with was to call a
  P/Invoke).

Test case (size of the main executable): link all (debug)

* Before:  33.522.704 bytes
* After:   33.506.112 bytes
* Difference: -16.592 bytes (-0.05 %)

There were no size differences in release mode, nor were there any size
differences in the "don't link" test, neither for debug nor release mode.

Fixes xamarin#5518.
@rolfbjarne rolfbjarne mentioned this issue Feb 4, 2022
Merged
@rolfbjarne rolfbjarne self-assigned this Feb 4, 2022
rolfbjarne added a commit that referenced this issue Feb 7, 2022
@rolfbjarne
[runtime] Make Runtime.Arch a readonly field in .NET. Fixes #5518. (#…
42c87ab 
…14076)

* Make Runtime.Arch a readonly field.
* Tell the AOT compiler Runtime.Arch is a constant value.
* Tell the linker to stub out the method we use to fetch the current
  architecture from native code (it turned out a bit complicated to set the
  Arch field when it's readonly - the solution I came up with was to call a
  P/Invoke).

Test case (size of the main executable): link all (debug)

* Before:  33.522.704 bytes
* After:   33.506.112 bytes
* Difference: -16.592 bytes (-0.05 %)

There were no size differences in release mode, nor were there any size
differences in the "don't link" test, neither for debug nor release mode.

Fixes #5518.
@ghost ghost locked as resolved and limited conversation to collaborators May 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@rolfbjarne rolfbjarne

Labels
breaking-change If an issue or a pull request represents a breaking change enhancement The issue or pull request is an enhancement iOS Issues affecting Xamarin.iOS
Projects
None yet
Milestone
XAMCORE_4_0
Development

Successfully merging a pull request may close this issue.

[runtime] Make Runtime.Arch a readonly field in .NET. Fixes #5518. rolfbjarne/xamarin-macios
2 participants
@rolfbjarne @spouliot