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

Overload for Make (IntPtr, ...) #7

Open
Therzok opened this issue Jun 3, 2017 · 9 comments
Open

Overload for Make (IntPtr, ...) #7

Therzok opened this issue Jun 3, 2017 · 9 comments

Comments

@Therzok
Copy link
Contributor

Therzok commented Jun 3, 2017

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?

@migueldeicaza
Copy link
Collaborator

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
Copy link
Collaborator

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.

@Therzok
Copy link
Contributor Author

Therzok commented Jun 3, 2017

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
Copy link
Contributor Author

Therzok commented Jun 3, 2017

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
Copy link
Contributor Author

Therzok commented Jun 3, 2017

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
Copy link
Contributor Author

Therzok commented Jun 3, 2017

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

@migueldeicaza
Copy link
Collaborator

Ugh. I do not like it.

@Therzok
Copy link
Contributor Author

Therzok commented Jun 3, 2017

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 is essential, it's just that the user needs to know to cache the delegate

@migueldeicaza
Copy link
Collaborator

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.

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

No branches or pull requests

2 participants