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

Updates codebase to support Medium/Partial trust #149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Shazwazza
Copy link

We had read somewhere that PetaPoco was supposed to support partial trust but unfortunately this is not the case. The problem is in only one part of the code in the CreateMultiPocoFactory method. This method creates a DynamicMethod that is not medium trust compatible. There are only 2 overloads that work for creating a DynamicMethod in medium trust and those are the overloads that don't set an 'owner' for the method. By not setting an owner, the method is essentially a static anonymously hosted method. It turns out that this DynamicMethod in particular is actually not required and therefore the IL Emit code is not required either. I did have it working with IL Emit codes in medium trust before I realized this so I still have that code saved but it is much nicer (and probably better performance wise) to not have to use IL Emit.

An overview of the changes made:

  • changes CreateMultiPocoFactory, this is the problem with medium trust... you cannot declare a DynamicMethod with an 'owner' in medium trust. Also, there there is no need for any of the IL.Emit logic in this method and therefore no need for the DynamicMethod at all in this method. It has been changed so that the MultiPocoFactory class does the heavy lifting. Without a DynamicMethod or IL Emit, this should work a teeny bit faster and is much easier to debug. Before I realized that the IL Emit was not necessary, i did make it work in medium trust with IL Emit so have saved that code locally in case its required.
  • changes some 'object' references to 'Delegate' (easier to understand)
  • changes MultiPocoFactory class to perform the operation of calling the callback and calling each mapper delegate

- changes some 'object' references to 'Delegate' (easier to understand)
- changes MultiPocoFactory class to perform the operation of calling the callback and calling each mapper delegate
- changes CreateMultiPocoFactory, this is the problem with medium trust... you cannot declare a DynamicMethod with an 'owner' in medium trust. Also, there there is no need for any of the IL.Emit logic in this method and therefore no need for the DynamicMethod at all in this method. It has been changed so that the MultiPocoFactory class does the heavy lifting. Without a DynamicMethod or IL Emit, this should work a teeny bit faster and is much easier to debug. Before I realized that the IL Emit was not necessary, i did make it work in medium trust with IL Emit so have saved that code locally in case its required.
@pleb
Copy link
Member

pleb commented Dec 4, 2015

Hi There. Thanks for the contribution. You're targeting v4. We're about to make v5 the standard, and as such, we not going to process this PR.

@pleb pleb closed this Dec 4, 2015
@Shazwazza
Copy link
Author

The thing about this change though is that in theory it is a performance optimization. This change essentially means there is less code generation required because the actual change means that a statically declared method is used instead of the same method being generated each time.

We've been using this change in Umbraco core for years now. I've not actually run the benchmarks on this change as compared to the original code, so I'm not sure how much perf optimization there is but there's definitely less codegen going on.

@pleb pleb reopened this Dec 14, 2015
@pleb
Copy link
Member

pleb commented Dec 14, 2015

I've reopened the PR. Thanks for the explanation.

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.

2 participants