Move _p2t_edge_list into a WeakMap on the SweepContext

[rcfox]

When WeakMap is available, push the _p2t_edge_list Point property into a WeakMap on the SweepContext. This prevents creating a new hidden class when using a custom Point class, and allows for the edge lists to be garbage collected up with the SweepContext.

The basic functionality of WeakMap is supported by pretty much everything. However, it is not supported by PhantomJS, so the current behaviour of adding _p2t_edge_list to Points is maintained if WeakMap is not available.

[r3mi]

Thanks for the patch. I like the idea of not modifying the points if possible.

However, the current proposal has a significant performance impact compared to previous versions (between -30% and -35% on my test machine, using npm run bench). May be making it optionnal ? There is an options argument in the SweepContext constructor. So handling e.g. options.frozenPoints.

There should also be a test case to check that points are not modified, depending on the option.

[rcfox]

So I'm not sure the benchmark reflects actual usage. The _p2t_edge_list being defined on the Point class seems to make a big difference. Do you think most people use the poly2tri Point class, or their own?

If you replace this line in makePoints() in benchmark.js (in master):

points.push(new Point(a[i], a[i + 1]));

with:

 points.push({x: a[i], y: a[i+1]});

Then the performance is pretty much the same as with my change for me. If you do:

 points.push({x: a[i], y: a[i+1], _p2t_edge_list: null});

Then you get back that performance boost.

---

In the case of enabling the option to use a WeakMap, how terrible would it be to do something like this?

var SweepContext = function(contour, options) {
   // ...
    Point.add_edge_list = !options.frozenPointClass
    // ...
}


var Point = function(x, y) {
    // ...
    if (Point.add_edge_list) {
        /**
         * The edges this point constitutes an upper ending point
         * @private
         * @type {Array.<Edge>}
         */
        this._p2t_edge_list = null;
    }
};

Point.add_edge_list = true;

My main issue is that when you're holding onto 2-3 million points, every extra property removed starts to make a difference.

[rcfox]

Actually, it would probably make more sense to accept a point factory with a default one if none is given.

[r3mi]

I ran the benchmark again, as per your suggestion, and found that benchmark with Point and {x, y, _p2t_edge_list} are 25% faster than {x, y}. I think this has to do with the JIT compiler : code which use objects with the same "shape" can be better optimized (see Hidden Classes for example). This is interesting, I didn't measure this effect before. I think I will update the benchmark.js to keep it tested.

Still thinking about a good solution with all constraints.

[r3mi]

My thoughts so far:

1. in my tests, the WeakMap branch impacts the performance (about 30%), in all scenarios : Point, {x,y}, {x,y, _p2t_edge_list} (the master branch now contains the updated benchmark.js to test these 3 scenarios).
2. on the other hand, the WeakMap branch should help garbage collecting

From 1. : I want the default poly2tri behavior to provide the best performances, so

• The poly2tri.Point class should keep its current definition, with _p2t_edge_list unconditionally defined (to help the JIT compiler)
• The WeakMap behavior should be optional

From 2. : I need to find a way to unit test the improved garbage collecting behavior. Currently looking into it.

[r3mi]

Thanks.

Thinking about this discussion, we have 2 different issues :

1. there is a performance penalty in using {x,y} compared to Point ;
2. the library is not GC-friendly : after triangulation, it is difficult to free the allocated memory due to the cross-references between the data structures.

I have no solution for 1. for the time being. For 2. , I suspect the current patch is not sufficient. I am looking into it, especially writing some tests to measure memory leaks.

[rcfox]

The only GC issue I saw was that Edge objects were being tacked on to my application's Point objects, which live for a long time.

For me, the SweepContext has a short life time. If all of the objects are only referenced by it, then everything should get GC'd with it.