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

[RFC] Simplify block creation pattern #4543

Closed
rolfbjarne opened this issue Aug 1, 2018 · 4 comments
Closed

[RFC] Simplify block creation pattern #4543

rolfbjarne opened this issue Aug 1, 2018 · 4 comments
Labels
enhancement The issue or pull request is an enhancement iOS Issues affecting Xamarin.iOS macOS Issues affecting Xamarin.Mac request-for-comments The issue is a suggested idea seeking feedback
Milestone

Comments

@rolfbjarne
Copy link
Member

The current pattern generated by the generator (and which we copy around liberally), goes something like this:

unsafe {
	BlockLiteral *block_ptr_handler;
	BlockLiteral block_handler;
	block_handler = new BlockLiteral ();
	block_ptr_handler = &block_handler;
	block_handler.SetupBlockUnsafe (blockCallback, userCallback);

	NativePInvoke ((void*) block_ptr_handler);
	block_ptr_handler->CleanupBlock ();
}

I think we can make it possible to simplify this a lot, so that we get down to:

using (var block = new BlockLiteral (blockCallback, userCallback, false))
    NativePInvoke (ref block);

This requires:

  • Make BlockLiteral implement IDisposable (and call CleanupBlock in Dispose).
  • Add a BlockLiteral constructor that takes the corresponding delegates.
  • Add support to the linker/optimizer for the new BlockLiteral constructor.
  • Change P/Invokes to take ref BlockLiteral instead of void * or IntPtr (this only works if the block isn't nullable).

Advantages

  • Much smaller & simpler C# code.
  • We won't leak the block if the P/Invoke (or any code after setting up the block) throws an exception.

Disadvantages

  • I think we'd end up creating more compiled (both IL and native), because of the additional exception handling by the using statement. This would need to be confirmed by testing though.
@rolfbjarne rolfbjarne added the enhancement The issue or pull request is an enhancement label Aug 1, 2018
@rolfbjarne rolfbjarne added this to the Future milestone Aug 1, 2018
@spouliot
Copy link
Contributor

spouliot commented Aug 1, 2018

I think it's easy to measure the potential disadvantage (if any) before implementing the generator/linker changes.

Another advantage is that we sometimes needs to use the blocks in non-generated (manual) code. Hiding most details will reduce potential bugs and ease maintainability (right now a required change would need a lot of sites to fix).

@spouliot spouliot added macOS Issues affecting Xamarin.Mac iOS Issues affecting Xamarin.iOS labels Aug 1, 2018
@spouliot
Copy link
Contributor

spouliot commented Aug 1, 2018

Testing-wise we should ensure that, even if easier, manual implementation are optimizable (if decorated as such). In fact we might want to do that in parallel (or without the RFC) since this is an existing situation (even if they all work today it's hard to guarantee they will remain that way).
#4547 (review)

@rolfbjarne
Copy link
Member Author

we should ensure that manual implementation are optimizable

All block creations in our platform assemblies must be optimizable, otherwise we can't safely remove the dynamic registrar.

This is enforced by tests.

The only way to ensure that any optimization is working correctly, is to execute the optimized code. Which is one of the reasons we need to write tests (monotouch-tests) for all manually bound code 😄 (we execute monotouch-test both in optimized and non-optimized mode).

@rolfbjarne
Copy link
Member Author

Duplicate of #15783.

@rolfbjarne rolfbjarne closed this as not planned Won't fix, can't repro, duplicate, stale Sep 23, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement The issue or pull request is an enhancement iOS Issues affecting Xamarin.iOS macOS Issues affecting Xamarin.Mac request-for-comments The issue is a suggested idea seeking feedback
Projects
None yet
Development

No branches or pull requests

2 participants