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

Remove boxing from value to TValue in FastMemberInvoker #1450

Open
GeertvanHorrik opened this issue Oct 22, 2019 · 29 comments
Assignees
Labels
Milestone

Comments

@GeertvanHorrik
Copy link
Member

@GeertvanHorrik GeertvanHorrik commented Oct 22, 2019

@GeertvanHorrik GeertvanHorrik added this to the 5.12.0 milestone Oct 22, 2019
@GeertvanHorrik GeertvanHorrik self-assigned this Oct 22, 2019
@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 22, 2019

@GeertvanHorrik I'm looking a bit more in-depth at that example and thinking there may be a better way approaching it via an IConvertible path but I need to play some games, I'll let you know what I find.

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 22, 2019

Scratch that endeavor, I forgot that since primitives are value types they'll box if you cast to an interface. Back to the drawing board.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 22, 2019

Just pushed a large update (to fix the unit tests I broke last evening). Here is something that we might be able to use as well:

https://github.com/Catel/Catel/pull/1451/files#diff-58d9dfff4a846f6a8359f60f50453882R16

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 22, 2019

So for setting, we are already using Convert.To[Type] to convert without a hassle (see https://github.com/Catel/Catel/pull/1451/files#diff-4b2a4613038151d245c7752e88f552d9R28). So the only thing is converting back, where we try to use the boxing cache if possible.

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 22, 2019

Interesting, I'll take a look at it and break down the IL code, see if it will do what we want.

@GeertvanHorrik GeertvanHorrik changed the title Removing boxing from value to TValue in FastMemberInvoker Remove boxing from value to TValue in FastMemberInvoker Oct 22, 2019
@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 22, 2019

So I might need some breadcrumbs here because I'm getting lost in the context. Is the conversion back boxing because it's all being stored as object or is it something else. I believe generics also have to do boxing for various operations but I'm not sure this is it.

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 22, 2019

@GeertvanHorrik This also may have some promise: https://stackoverflow.com/a/12386123

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 22, 2019

The only thing that doesn’t use strings is the undocumented thing, but that does look promising indeed!

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 22, 2019

So the modelbase already takes care of the boxing in 5.12. The only thing left is that we need to box the value when getting it because we cannot cast a bool to TValue, even if we know it's a bool. So we need (TValue)(object)myBoolValue

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 22, 2019

Right now I'm with you, so that may be an issue then. My understanding is that casting anything from a value type to a generic requires boxing because of how generics (basically value type -> object -> tvalue) behave under the covers. That may be a pain point that has to be lived unless there's something I'm missing here and I usually like to avoid using IL Emit to play games.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 24, 2019

@vaecors this might be interesting, especially if it can be combined with T4 templates:

https://stackoverflow.com/a/7686838/561411

Will investigate a bit more this weekend.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 24, 2019

Some more info: http://benbowen.blog/post/fun_with_makeref/

Big warning from what I understand when quickly scanning the post: only apply to value types, never to reference types (but in that case boxing doesn't occur, so that's fine).

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 24, 2019

And this is exactly what we are trying to achieve:

http://steven.thuriot.be/generic-references/

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 24, 2019

@GeertvanHorrik There shouldn't be any reason why it wouldn't (ignoring his rogue exception being thrown). The only thing that makes me uncomfortable about it is the fact it uses some undocumented keywords and it will require research as to what target platforms/frameworks it actually supports.

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 24, 2019

It looks like Mono and Xamarin support it,there were some bugs about it last year not properly behaving but it appears to be fixed. Some deeper reader shows they're doing pointer manipulation but don't require the unsafe keyword? I'm not sure what that might introduce.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 24, 2019

I think we can fix this in a generic way (e.g. BoxingCache<T>.CastWithoutBoxing(T value)) so we could technically use this everywhere. Not sure yet, will digest it for a while, now trying to replace all the PropertyHelper calls in Catel with the new ObjectAdapter (see #1454 )

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 24, 2019

Yeah it's definitely something worth looking into. I just have concerns that it's behavior is largely undocumented and there's hints that it's doing pointer manipulation that may or may not require wrapping in an unsafe to avoid GC issues. May be wise to develop a PoC outside of the scope of Catel just to stress test it and see what it's actually doing.

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 24, 2019

@GeertvanHorrik Here's the IL from that Convert example, it looks like it may be OK:

Convert<T>:
IL_0000:  nop         
IL_0001:  ldtoken     UserQuery.T
IL_0006:  call        System.Type.GetTypeFromHandle
IL_000B:  ldtoken     System.Int32
IL_0010:  call        System.Type.GetTypeFromHandle
IL_0015:  call        System.Type.op_Equality
IL_001A:  stloc.0     
IL_001B:  ldloc.0     
IL_001C:  brfalse.s   IL_0037
IL_001E:  nop         
IL_001F:  ldc.i4.5    
IL_0020:  stloc.1     
IL_0021:  ldloca.s    01 
IL_0023:  mkrefany    System.Int32
IL_0028:  refanyval   UserQuery.T
IL_002D:  ldobj       UserQuery.T
IL_0032:  stloc.2     
IL_0033:  ldloc.2     
IL_0034:  stloc.3     
IL_0035:  br.s        IL_0080
IL_0037:  ldtoken     UserQuery.T
IL_003C:  call        System.Type.GetTypeFromHandle
IL_0041:  ldtoken     System.Int64
IL_0046:  call        System.Type.GetTypeFromHandle
IL_004B:  call        System.Type.op_Equality
IL_0050:  stloc.s     04 
IL_0052:  ldloc.s     04 
IL_0054:  brfalse.s   IL_0073
IL_0056:  nop         
IL_0057:  ldc.i4.6    
IL_0058:  conv.i8     
IL_0059:  stloc.s     05 
IL_005B:  ldloca.s    05 
IL_005D:  mkrefany    System.Int64
IL_0062:  refanyval   UserQuery.T
IL_0067:  ldobj       UserQuery.T
IL_006C:  stloc.s     06 
IL_006E:  ldloc.s     06 
IL_0070:  stloc.3     
IL_0071:  br.s        IL_0080
IL_0073:  ldloca.s    07 
IL_0075:  initobj     UserQuery.T
IL_007B:  ldloc.s     07 
IL_007D:  stloc.3     
IL_007E:  br.s        IL_0080
IL_0080:  ldloc.3     
IL_0081:  ret         
@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 25, 2019

Made me think, we could revive this ticket: #1319 if this works well. Not sure if it's much slower though. So there is the question what is harder for memory:

  1. Separate dictionaries per (value) type
    -or-
  2. Boxing the values (using BoxingCache, thus re-using boxed values)
@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 25, 2019

Did some benchmarking, looks pretty fast:

image

This is a property bag that will store each value type in the right dictionary. I think especially looking up is expensive, not the actual "casting".

NonTyped: dictionary<string, object>
Typed: dictionary per type, but preventing boxing with cached functions
SuperTyped: dictionary per type, but using __refvalue and __makeref

Both typed versions prevent boxing, but the functions are just too slow. The SuperTyped though seems fast enough to consider it.

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 25, 2019

It would probably be a good idea then to setup a benhmark that would simulate a use case and run it through dotMemory (or similar) to see what the GC is doing or there are any leaks.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 25, 2019

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 25, 2019

What I can do for you is when I get into the office I’ll pull that repo down and run it through dotMemory and dump some reports, sound good?

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 25, 2019

Great idea! So each property bag will use more memory (because dictionary per type), but we could lazy instantiate to prevent most of it. Then I think we have nearly the same performance but without the boxing.

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 25, 2019

image

This is a general dotMemory profile, I could make it more granular but that may take some time as I have to write conditional snapshots into the code. Let me know if this will work as a general idea for you though.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 25, 2019

As long as we know it per benchmark, I am already happy and we at least have some results.

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 25, 2019

This is basically an average mean between all 3 of your benchmarks. But I agree this looks very promising and will definitely be a nice boost overall. We'll definitely notice it as we have some datagrids with 500,000+ rows of data or more.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 28, 2019

I am still thinking about this. In this benchmark, we don't yet take into account the boxing cache that we already have. Will need to create a more advanced benchmark (including memory management).

@vaecors

This comment has been minimized.

Copy link
Contributor

@vaecors vaecors commented Oct 28, 2019

@GeertvanHorrik I agree wholeheartedly with this. I think a good pathway would be to really breakdown the BenchmarkDotNet tests to more specific units of work. Memory management analysis could be more granulated using the dotMemory profiling api (if you have access to this) where it can be broken down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.