Add a per member option DoNotInline #1876

Merged
merged 6 commits into from Jan 3, 2017

Projects

None yet

3 participants

@lbargaoanu
Contributor

From #1794 (comment).
It does happen without collections too. It's about how big the execution plan becomes. I guess you should pick and choose where you don't inline, but I didn't bother with that here.

@lbargaoanu lbargaoanu changed the title from Add a per member option DontInline to Add a per member option DoNotInline Jan 2, 2017
@TylerCarlson1
Member

Is there any metric of how much faster it is on first map? I would think it wouldn't be all that much faster if you have an item with all its collections filled as the first map. Because you will have to make all the maps anyway it's just in pieces instead of all at once.

Also there's going to be a performance hit for every item in the list cause you will have to find the right map func in the typemapplan cache. It's a give and take on performance I know. I'm just curious of how much benefit it actually gives.

@lbargaoanu
Contributor

Huge. Try the tests for yourself. It's not linear, after a certain code size, stuff hits the fan :) The depth of it might make things worse.

@TylerCarlson1
Member

ok thanks that's all I really wanted to know. I'll take your word on it :)

@jbogard
Member
jbogard commented Jan 3, 2017

This one good to go?

@lbargaoanu
Contributor

Yes.

@jbogard jbogard added the Feature label Jan 3, 2017
@jbogard

That property marked internal - is that necessary?

src/AutoMapper/PropertyMap.cs
@@ -32,6 +31,7 @@ public PropertyMap(PropertyMap inheritedMappedProperty, TypeMap typeMap)
public IEnumerable<MemberInfo> SourceMembers => _memberChain;
+ public bool Inline { get; internal set; } = true;
@jbogard
jbogard Jan 3, 2017 Member

I don't think there's a need to make this internal - none of the other properties are?

@lbargaoanu
lbargaoanu Jan 3, 2017 Contributor

Yes, I meant to do it and forgot.

@lbargaoanu
lbargaoanu Jan 3, 2017 Contributor

Done.

@jbogard
jbogard approved these changes Jan 3, 2017 View changes
@jbogard jbogard added this to the vNext milestone Jan 3, 2017
@jbogard jbogard merged commit e5cde15 into AutoMapper:master Jan 3, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@lbargaoanu lbargaoanu deleted the lbargaoanu:DontInline branch Jan 3, 2017
@lbargaoanu
Contributor

For future reference. Another approach here, less ad-hoc, would be to use smth like max depth, with a reasonable default, maybe 10. The problem would then be that the context map thing would need to behave exactly as the inlined version. This doesn't happen today because of declared types/runtime types and because the configuration is not passed from the initial call to the context map call. Certainly more involved, but it would work by default. What we have now seems a reasonable compromise, but I was thinking about this and wanted to leave a record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment