NStack icon indicating copy to clipboard operation
NStack copied to clipboard

Overload for Make (IntPtr, ...)

Open Therzok opened this issue 8 years ago • 9 comments

Currently, there is: ustring Make (IntPtr block, int size, Action<ustring, IntPtr> releaseFunc = null)

The option to avoid allocating here is to make the caller cache the lambda at callsite, this can lead to lots of static delegates being declared, and possibly captures, etc. I'm not sure there is a better way to handle this though, so the users will have to deal with the delegate allocations.

It's not necessary to know the ustring that's being released. I don't have enough information to understand why we also pass in the ustring that's being reclaimed. Done this way, the user can still shoot themselves in the foot, since they don't know the implementation details of IntPtrUString, thus if they decide to use the string after free-ing the IntPtr, it'll lead to invalid memory accesses.

Maybe trim down the Action parameters to IntPtr? That way, you can use built-in methods like Marshal.FreeHGlobal and use it as a method group at call-site?

Therzok avatar Jun 03 '17 00:06 Therzok

Good observation o n the Action, will do.

Also, I think I need an overload for "Make me a ustring from a buffer, but make a copy of the data, and you take care of things"

migueldeicaza avatar Jun 03 '17 02:06 migueldeicaza

I do not think that we need to pass a delegate, you could pass a static method, and that should work, right?

static void MyFree (IntPtr block){}

ustring.Make (.., MyFree);

That said, I realized another idiom I want to support "Make me a ustring from a zero-terminated string" with and without deallocation, makes sense to have an Action-less version.

migueldeicaza avatar Jun 03 '17 02:06 migueldeicaza

And that's where you get bit by the problem. Static methods don't guarantee that. See IL_000a.

  .method public hidebysig static class NStack.ustring 
          MyMake(native int block,
                 int32 size) cil managed
  {
    // Code size       25 (0x19)
    .maxstack  4
    .locals init (class NStack.ustring V_0)
    IL_0000:  nop
    IL_0001:  ldarg.0
    IL_0002:  ldarg.1
    IL_0003:  ldnull
    IL_0004:  ldftn      void NStack.ustring::MyFree(native int)
    IL_000a:  newobj     instance void class [System.Runtime]System.Action`1<native int>::.ctor(object,
                                                                                                native int)
    IL_000f:  call       class NStack.ustring NStack.ustring::Make(native int,
                                                                   int32,
                                                                   class [System.Runtime]System.Action`1<native int>)
    IL_0014:  stloc.0
    IL_0015:  br.s       IL_0017

    IL_0017:  ldloc.0
    IL_0018:  ret
  } // end of method ustring::MyMake

Therzok avatar Jun 03 '17 02:06 Therzok

I'm not sure whether this is optimized at runtime in the use-case of method groups like the MyFree in your example, I have to test under a profiler and find out, but this is the IL generated by the compiler.

Therzok avatar Jun 03 '17 02:06 Therzok

~Funnily, this is better optimized:~ I can't read.

   .method assembly hidebysig instance void 
            '<MyMake>b__10_0'(native int ptr) cil managed
    {
      // Code size       8 (0x8)
      .maxstack  8
      IL_0000:  ldarg.1
      IL_0001:  call       void NStack.ustring::MyFree(native int)
      IL_0006:  nop
      IL_0007:  ret
    } // end of method '<>c'::'<MyMake>b__10_0'

  } // end of class '<>c'

  .method public hidebysig static class NStack.ustring 
          MyMake(native int block,
                 int32 size) cil managed
  {
    // Code size       44 (0x2c)
    .maxstack  4
    .locals init (class NStack.ustring V_0)
    IL_0000:  nop
    IL_0001:  ldarg.0
    IL_0002:  ldarg.1
    IL_0003:  ldsfld     class [System.Runtime]System.Action`1<native int> NStack.ustring/'<>c'::'<>9__10_0'
    IL_0008:  dup
    IL_0009:  brtrue.s   IL_0022

    IL_000b:  pop
    IL_000c:  ldsfld     class NStack.ustring/'<>c' NStack.ustring/'<>c'::'<>9'
    IL_0011:  ldftn      instance void NStack.ustring/'<>c'::'<MyMake>b__10_0'(native int)
    IL_0017:  newobj     instance void class [System.Runtime]System.Action`1<native int>::.ctor(object,
                                                                                                native int)
    IL_001c:  dup
    IL_001d:  stsfld     class [System.Runtime]System.Action`1<native int> NStack.ustring/'<>c'::'<>9__10_0'
    IL_0022:  call       class NStack.ustring NStack.ustring::Make(native int,
                                                                   int32,
                                                                   class [System.Runtime]System.Action`1<native int>)
    IL_0027:  stloc.0
    IL_0028:  br.s       IL_002a

    IL_002a:  ldloc.0
    IL_002b:  ret
  } // end of method ustring::MyMake
		static void MyFree (IntPtr block) {}
		public static ustring MyMake (IntPtr block, int size)
		{
			return Make (block, size, ptr => MyFree (ptr));
		}

Therzok avatar Jun 03 '17 02:06 Therzok

Okay, not a big problem, if people use a lambda without captures, it's a-ok.

Therzok avatar Jun 03 '17 02:06 Therzok

Ugh. I do not like it.

migueldeicaza avatar Jun 03 '17 02:06 migueldeicaza

~The lambda variant is indeed more efficient - one action allocated per call-site.~ I can't read, sorry, both allocate.

I do not see a better way to implement the native memory handling. Keeping Action<IntPtr> is essential, it's just that the user needs to know to cache the delegate

Therzok avatar Jun 03 '17 02:06 Therzok

Another option is to have a base class that does not release, and a couple of subclasses: one that releases with Marshal.Free, and another one that releases with an Action.

migueldeicaza avatar Jun 25 '17 02:06 migueldeicaza