Huge performance drawdown since version 1.5.1

[RomanHotsiy]

Hi @mdaines. First, thank you for your hard work!

We work with big graphs and noticed a huge performance drawdown since version 1.5.1.

I have prepared a plunkr in which I measured time to build one of our sample graphs using 3 different versions of viz.js: 1.4.1, 1.5.1 and 1.6.0. On my machine (MacOS Sierra, Chrome 55) I've got the following results:

First run:

  Viz.js 1.4.1: 231.66949999999997ms
  Viz.js 1.5.1: 1236.600000000001ms
  Viz.js 1.6.0: 825.5665000000001ms

Second run:

viz.js 1.4.1: 66.71800000000003ms
viz.js 1.5.1: 660.3394999999997ms
viz.js 1.6.0: 454.093ms

You can check it yourself here: http://plnkr.co/edit/g4h7F2wfoU3MV1YmiIie?p=preview.

Most likely this was caused by switching from -O2 to -Os or by enabling dynamically allocated memory ALLOW_MEMORY_GROWTH=1 (they say that enabling this option disables some optimizations)

I didn't manage to install emscripten on my machine in a reasonable amount of time so wasn't able to check the exact reason.

I have attached a few graphs we work with in dot format to this issue.

Thank you.

dot.zip

[mdaines]

This was probably introduced in 1.5.0. In order to address #65 and #59, I changed Viz.js to create a new Emscripten module instance on every call (somewhat like running the dot program over and over, as opposed to using Graphviz as a library). The issues I linked have more detail, but there were two problems with reusing the module instance:

• Certain kinds of syntax errors could cause subsequent calls to fail.
• The gvFreeLayout call would abort for certain graphs, even though they would render successfully.

As well as the overhead of setting up a module instance, there is probably some performance impact from the extra garbage collection involved. And even though I was attempting to prioritize stability over performance for that release, in some situations Viz.js can now cause an out of memory error. See #72.

There may be workarounds that allow Viz.js to reuse the module instance:

• Viz.js could try to call gvFreeLayout after rendering and returning the result. If gvFreeLayout aborts, a new module instance could be created.
• Similarly, a new module instance could be created if an error occurs.

I'll look at implementing these for a patch release.

[RomanHotsiy]

@mdaines I have run the same test agains the much bigger dot file and this definitely doesn't look as an overhead caused by setting up module instances as it relates too much on graph size. For the github graph I attached to the previous issue I get the following numbers:

First run:

Viz.js 1.4.1: 11277.684999999998ms
Viz.js 1.5.1: 22888.15000000001ms
Viz.js 1.6.0: 14689.070000000007ms

Second run:

Viz.js 1.4.1: 8590.240000000005ms
Viz.js 1.5.1: 23138.10499999998ms
Viz.js 1.6.0: 14433.345000000001ms

Plunkr is here

This time I run generation only once. As you can see difference between 1.4.1 and 1.5.1 is more than 11seconds which doesn't looks like as an overhead of setting up a module and garbage collection.

I'm pretty sure this was caused either by switching from -O2 to -Os or by enabling dynamically allocated memory ALLOW_MEMORY_GROWTH=1.
I didn't manage to install emscripten on my Mac so I can't check the exact issue.

I can prepare a performance test as a PR and you then can run it with different options.
Or if this doesn't work for you, you build and post somewhere two different bundles from latest:

• one with -O2 instead of -Os
• one with ALLOW_MEMORY_GROWTH=1 disabled

I will run tests myself and let you know if any of above is the reason.

thanks

[mdaines]

Thanks for checking a larger graph -- I'll try modifying those options to see if there's any improvement and post the results here.

[mdaines]

Here are my results (using Node). It certainly looks like ALLOW_MEMORY_GROWTH slows things down, and that the -Os setting might be slightly faster. I've attached the builds I used below.

N = 10
ALLOW_MEMORY_GROWTH=1, Os:  6952.7
ALLOW_MEMORY_GROWTH=0, Os:  2849.3
ALLOW_MEMORY_GROWTH=1, O2:  6516.2
ALLOW_MEMORY_GROWTH=0, O2:  3487.6

Viz-1.6-issue-74.zip

[RomanHotsiy]

Thanks for your efforts!

It would be great if we can fix this issue.

I see the point why you enabled ALLOW_MEMORY_GROWTH but believe we should disable it. Most of users work with small graphs so this setting doesn't change anything for them. But those who may be affected by memory limit struggle more because of bad performance. Our graph which caused out-of-memory crash now takes more than 80 seconds to be generated (which is unusable 😞 )

How about bringing back setMemorySize introduced by my college in 506ca79

I understand API-wise it is not so good for users to guess memory-size required but we can't come up with a better solution.

What do you think?

[mdaines]

My hunch also is that most people don't run into that problem. I could add an option to the Viz() function, like so:

Viz(src, { totalMemory: 32 * 1024 * 1024 })

[RomanHotsiy]

@mdaines it would be perfect!