GL.getNewId memory leak fix

[mifwarior]

In the company we make an editor, and therefore there is a need to create a lot of buffers at runtime.
This fix is used to remove the memory leak, since the current implementation on the array increases it indefinitely.

cloud.opendesign.com

[kripken]

This can save some memory if you do many allocations here, but it will also be slower as accessing objects is slower than arrays.

I'd suggest looking at HandleAllocator in src/library.js, which uses a fast array with a free-list to avoid leaking. That might be a better approach here.

[mifwarior]

The first one we used is:

  getNewId: function(table) {
      GL.counter++;
      if(table.index === undefined)  table.index = 0
      const maxSize = Math.max(table.length, 1)
      table.index = (table.index + 1) % maxSize

      for (var i = 0; i < maxSize; i++) {
          let index = (table.index + i) % maxSize;
          
          if (!table[index]) {
              table.index = index
              table[index] = null;
              return index
         }
      } 

      table.index = table.length;
      table[table.index] = null;
 
   return table.index;
  }

but there was a problem that the array remained in size with the peak number of objects.
This partially solved the problem.

Then next step we tried using new Map:

        var GL = {
          counter: 1,
          buffers: new Map(),
          programs: new Map(),
          framebuffers: new Map(),
          renderbuffers: new Map(),
          ....
        }

      ....

        getNewId: function (table) {
          return GL.counter++;
        }

      ....

          function __glGenObject(n, buffers, createFunction, objectTable) {
          for (var i = 0; i < n; i++) {
            var buffer = GLctx[createFunction]();
            var id = buffer && GL.getNewId();

            if (buffer) {
                buffer.name = id;
                objectTable.set(id, buffer)
                HEAP32[buffers + i * 4 >>> 2] = id;
            } else {
                GL.recordError(1282);
            }
          }
        }

    ....

        function _glDeleteBuffers(n, buffers) {
            for (var i = 0; i < n; i++) {
                var id = HEAP32[buffers + i * 4 >>> 2];
                var buffer = GL.buffers.get(id);
                if (!buffer) continue;
                GLctx.deleteBuffer(buffer);
                //buffer.name = 0;
                GL.buffers.delete(id);
            }
        }

but for some reason in the final js file was:

var GL = {
  counter: 1,
  buffers: {},
  programs: {},
  framebuffers: {},
  renderbuffers: {},
  ....
}

regardless, if we make changes with new Map() in the resulting file,
then get maximum performance and solve the memory problem.

After that, we had no choice in favor of slow objects

Thanks for suggesting to try with HandleAllocator, I'll check if it can help us.

[sbc100]

We have a recently-added thing called HandleAllocator which may be what we want to use here:

emscripten/src/library.js

Line 3737 in 30ba0ae

$HandleAllocator__docs: '/** @constructor */',

We use it in a few other places where we need to map JS objects to unique IDs.

[sbc100]

(Oops, I see @kripken already suggested that).

[sbc100]

BTW, HandleAllocator doesn't actually shink the underlying array when stuff gets free'd but it will populate it with null and then reuse those slots so you should not get unbounded growth or leaks.

[mifwarior]

@kripken @sbc100 Can we use "new Map()" for this optimization?

Can you please give an example of using handle allocator for GL object? Is there any documentation on macros\preprocess through which a GL object is generated?

[kripken]

new Map() is still slower than accessing an array. Maps can take more than constant time to look up a value.

For HandleAllocator, reading how it is used in existing code could be helpful.

[dborysov-ua]

``> new Map() is still slower than accessing an array. Maps can take more than constant time to look up a value.

For HandleAllocator, reading how it is used in existing code could be helpful.

In javascript array is not classic memory array like c++ or other language its like a js object with array like api
i am create a simple benchmark for operation get element by index use array object and map
`const arr = [];
const obj = {};
const map = new Map();

const value = { id: 0 };

for (let i = 0; i < 100000; i ++) {
arr[i] = value;
obj[i] = value;
map.set(i, obj);
}

{
const s = performance.now()
for (let i = 0; i < 1000000; i ++) {
arr[1000];
}
console.log('Array get element', performance.now() - s);
}

{
const s = performance.now()
for (let i = 0; i < 1000000; i ++) {
obj[1000];
}
console.log('Object get element', performance.now() - s);
}

{
const s = performance.now()
for (let i = 0; i < 1000000; i ++) {
map.get(1000);
}
console.log('Map get element', performance.now() - s);
}
`
Result for Chrome

Array get element 1.9424650073051453
Object get element 1.8631070107221603
Map get element 4.055746003985405

From which I conclude that getting by index for an object and an array is almost the same, and an object is even faster

[sbc100]

Regardless of how we micro-optimize the HandleAllocator, I think the its best to switch to the existing HandleAllocator for this purpose.

We can propose optimization to the handle allocator as a followup.

IIRC the issue you are trying to solve here is more about avoiding running out of handles, and the current HandleAllocator should solve that.

[dborysov-ua]

The main problem is that at the peak we have a large number of buffers, which then decreases to a small one, but at the same time a huge array full of zeros remains, which consumes memory, and the object solves this problem because it allows you to remove the element completely

[dborysov-ua]

HandleAllocator

Also, we could not figure out how to use HandleAllocator in your templates

Can you add a code example right in that file how to declare it correctly?

And another question, maybe it's worth using a variable to store all such objects and not a lot like now?

[dborysov-ua]

var GL = {
counter: 1,
buffers: new HandleAllocator(),
programs: new HandleAllocator(),
framebuffers: new HandleAllocator(),
renderbuffers: new HandleAllocator(),
....
}

i think need change to

GL = {
handleAllocator: new HandleAllocator()
}
and use id + 1 because 0 is not valid value for web GL

[mifwarior]

We tried using handleAllocator with the test above

const arr = [];
const obj = {};
const map = new Map();

function handleAllocator() {
  this.allocated = [];
  this.freelist = [];
  this.get = function(id) {
    return this.allocated[id];
  };
  this.allocate = function(handle) {
    let id;
    if (this.freelist.length > 0) {
      id = this.freelist.pop();
      this.allocated[id] = handle;
    } else {
      id = this.allocated.length;
      this.allocated.push(handle);
    }
    return id;
  };
  this.free = function(id) {
    delete this.allocated[id];
    this.freelist.push(id);
  };
}
const allocator = new handleAllocator()

const value = { id: 0 };

for (let i = 0; i < 100000; i ++) {
  arr[i] = value;
  obj[i] = value;
  map.set(i, obj);
  allocator.allocate(value)
}

{
  const s = performance.now()
  for (let i = 0; i < 1000000; i ++) {
    arr[1000];
  }
  console.log('Array get element', performance.now() - s);
}

{
  const s = performance.now()
  for (let i = 0; i < 1000000; i ++) {
    obj[1000];
  }
  console.log('Object get element', performance.now() - s);
}

{
  const s = performance.now()
  for (let i = 0; i < 1000000; i ++) {
    map.get(1000);
  }
  console.log('Map get element', performance.now() - s);
}

{
  const s = performance.now()
  for (let i = 0; i < 1000000; i ++) {
    allocator.get(1000);
  }
  console.log('HandleAllocator get element', performance.now() - s);
}

and get results:

Array get element 1.2000000001862645
Object get element 1.099999999627471
Map get element 2.2999999998137355
HandleAllocator get element 1.2999999998137355

@sbc100 @kripken What do you think, perhaps the test is not indicative? Then what test will be indicative?

As I noticed, you cannot insert objects created through the new operator into the macro instance.
And the following code will not work:

$GL : { 
    ...
    buffers: = new handleAllocator()
    ...
}

I investigated how many changes it would take to move to handleAllocator, and that's hundreds of changes.
When using an object, the number of changes in pr will be minimal, only 2 files.
Therefore the decision was to make through object.

An alternative would be to modify the getNewId function. Take the implementation of handleAllocator as an example and combine it with getNewId. What do you think about it?

[kripken]

Unfortunately tests like these may not work as you expect:

  for (let i = 0; i < 1000000; i ++) {
    obj[1000];
  }

First, the read from obj is not used anywhere, so it might be optimized out. Even if it isn't, it is the same in all iterations of the loop, so an optimization like loop invariant code motion will move it so it executes once outside of the loop.

To measure this carefully you'd need to use the result of the read, and store it somewhere it can't be optimized away, stuff like that.

[mifwarior]

@kripken Would it be nice if I change the pull request to use the handleAllocator?
But in this case there will be a lot of changes, since in GL an object cannot contain a handleAllocator instance.
Will it be ok?

What is the best way to choose for optimization?
I really want the pull request to be accepted, we need some kind of optimization.

[sbc100]

I took a stab at converting to HandleAllocator here: #18874. Still needs some more work but that is that general direction I think,