Fix issue #31

[andreasgal]

Remove release all, and release refcnt after allocation once a smart pointer holds the reference.

[andreasgal]

This still leaks a large amount of memory for my use case. I'll write a couple tests.

[mikeseven]

Thanks for the updates. The problem here is that you rely on v8 garbage collector to do cleanup. This is not a good idea because this (almost) never happens and that's the reason cl objects must be controlled out of v8. Also, in practice, GC happens rarely since it is a very low priority thread. So in high-performance OpenCL, where you have lots of events, you end up filling up GPU memory and typically crash it.

Node offers some control over GC and it used not be not reliable in v0.12 and before. I haven't experimented in v4+ and this might be what we want to use.

Again, keep in mind to release events often at runtime!

[andreasgal]

Not at all. The idea is to manually release events with cl.releaseEvent at runtime. If the user forgets that, the GC will do it. But the default is supposed to be cl.releaseEvent. Why would that not work? (thats what I am trying to do in my use case)

[andreasgal]

(I totally agree that relying on the GC is a bad idea. I have some JS code layered on top of this that tracks all events the API returns and frees them manually after every transaction.)

[andreasgal]

This test case leaks memory like crazy from the loop

'use strict';

var cl = require("./lib/opencl");

var ctx = cl.createContextFromType(
  [cl.CONTEXT_PLATFORM, cl.getPlatformIDs()[0]], cl.DEVICE_TYPE_CPU, null, null);

var device = cl.getContextInfo(ctx, cl.CONTEXT_DEVICES)[0];
var cq = cl.createCommandQueue(ctx, device, null);

var data = new Float32Array(16);
var b = cl.createBuffer(ctx, 0, data.buffer.byteLength);
while (true) {
  var ev = cl.enqueueWriteBuffer(cq, b, true, 0, data.buffer.byteLength, data, [], true);
  cl.releaseEvent(ev);
  cl.finish(cq);
}

cl.releaseMemObject(b);

cl.releaseCommandQueue(cq);
cl.releaseContext(ctx);

[andreasgal]

Simpler test case. Events leak.

'use strict';

var cl = require("./lib/opencl");
var ctx = cl.createContextFromType(
  [cl.CONTEXT_PLATFORM, cl.getPlatformIDs()[0]], cl.DEVICE_TYPE_CPU, null, null);

while (true) {
  var ev = cl.createUserEvent(ctx);
  cl.releaseEvent(ev);
}

[andreasgal]

Both of these allocations leak (this is a bindings problem, not a CL problem):

Nan::SetInternalFieldPointer(obj, 0, elm); Nan::SetInternalFieldPointer(obj, 1, new unsigned int(T::GetId()));

define NOCL_WRAP(T, V) \

NoCLWrapCLObject(new T(V), false)

[mikeseven]

Arghhh yes correct, they need to be manually release at v8 level. Thanks!

--mike

On Sat, Dec 5, 2015 at 11:42 AM -0800, "Andreas Gal" notifications@github.com<mailto:notifications@github.com> wrote:

Both of these allocations leak (this is a bindings problem, not a CL problem):

Nan::SetInternalFieldPointer(obj, 0, elm); Nan::SetInternalFieldPointer(obj, 1, new unsigned int(T::GetId()));

define NOCL_WRAP(T, V) \

NoCLWrapCLObject(new T(V), false)

Reply to this email directly or view it on GitHubhttps://github.com/mikeseven/node-opencl/pull/32#issuecomment-162240405.

[mikeseven]

you'll see a new branch where I have tried to resolve this memory leaks. At least for now, clRelease and even releaseAll seem to work correctly. The leak with the persistent is still there. I need to rearchitect.

I used your sample and instead of CL objects, I used some dummy JS objects. With node 5.0 and 5.1.1 on my mac, I do see the heap leaking even when forcing gc and giving it time to cleanup garbage. Do you see that too?

[andreasgal]

The GC heap probably increases some but with the CL object leak I eventually run out of heap whereas with general JS objects it levels out.

The right fix here is to use ObjectWrap from node or Nan and avoid custom allocations. I am a bit strapped for time but I can give it a try.

I also recommend removing NOCL_RELEASE_DRIVER_ISSUES. Instead for a broken driver I would just wrap the module in another module that overwrites cl.releaseMemObject etc and puts the requests in a list (all in JS), instead of doing this in the low-level node module. Wdyt?

[mikeseven]

Agree. In the new branch fwature/#31... I am resolving this issue. Originally I used object wrap but over time we changed it to trace driver leaks. Definitely a mistake, I'll rewrite and remove that flag.

--mike

On Mon, Dec 7, 2015 at 11:55 AM -0800, "Andreas Gal" notifications@github.com<mailto:notifications@github.com> wrote:

The GC heap probably increases some but with the CL object leak I eventually run out of heap whereas with general JS objects it levels out.

The right fix here is to use ObjectWrap from node or Nan and avoid custom allocations. I am a bit strapped for time but I can give it a try.

I also recommend removing NOCL_RELEASE_DRIVER_ISSUES. Instead for a broken driver I would just wrap the module in another module that overwrites cl.releaseMemObject etc and puts the requests in a list (all in JS), instead of doing this in the low-level node module. Wdyt?

Reply to this email directly or view it on GitHubhttps://github.com/mikeseven/node-opencl/pull/32#issuecomment-162640470.

[andreasgal]

@m1k3 can you review this? With this my node instance remains at 25M heap with the test case. I removed Equals(). Thats really surprising to overwrite. I added toString(), so if anyone needs == equality, .toString() === .toString() can be added on the proto of each wrapper in JS. If you want true identity, I would suggest using a membrane instead (track wrappers in a map of weak pointers, always wrap into the same object instance). Wdyt?

[mikeseven]

I'm testing it on other platforms.

--mike

On Tue, Dec 8, 2015 at 1:00 PM -0800, "Andreas Gal" notifications@github.com<mailto:notifications@github.com> wrote:

@m1k3https://github.com/m1k3 can you review this? With this my node instance remains at 25M heap with the test case. I removed Equals(). Thats really surprising to overwrite. I added toString(), so if anyone needs == equality, .toString() === .toString() can be added on the proto of each wrapper in JS. If you want true identity, I would suggest using a membrane instead (track wrappers in a map of weak pointers, always wrap into the same object instance). Wdyt?

Reply to this email directly or view it on GitHubhttps://github.com/mikeseven/node-opencl/pull/32#issuecomment-163016342.

[andreasgal]

(Happy to provide a membrane patch btw if you have a need for that)

[mikeseven]

Sure, fire up! Thanks.

--mike

On Tue, Dec 8, 2015 at 1:00 PM -0800, "Andreas Gal" notifications@github.com<mailto:notifications@github.com> wrote:

@m1k3https://github.com/m1k3 can you review this? With this my node instance remains at 25M heap with the test case. I removed Equals(). Thats really surprising to overwrite. I added toString(), so if anyone needs == equality, .toString() === .toString() can be added on the proto of each wrapper in JS. If you want true identity, I would suggest using a membrane instead (track wrappers in a map of weak pointers, always wrap into the same object instance). Wdyt?

Reply to this email directly or view it on GitHubhttps://github.com/mikeseven/node-opencl/pull/32#issuecomment-163016342.

[andreasgal]

Ok. Can you merge this one if it passes your tests? I'll open a new issue for the === equality patch.

[mikeseven]

excellent work, thanks a lot. This is great!