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

Memory access out of bounds error for graphviz layout #139

Closed
laurajunco opened this issue Dec 19, 2022 · 9 comments
Closed

Memory access out of bounds error for graphviz layout #139

laurajunco opened this issue Dec 19, 2022 · 9 comments

Comments

@laurajunco
Copy link

laurajunco commented Dec 19, 2022

Hi All, we are using Graphviz for creating graph layouts on an Angular application.

We've run into the issue that using the .layout() function leads to the error: Error: memory access out of bounds

It seems to appear after running a number of layout queries in a row, but it only seems to fail for json output.

I created a small test environment where it can be reproduced here: https://stackblitz.com/edit/wasm-memory-error . For the error click the button 4 times.

This is the code I'm running:

const graphviz = await Graphviz.load();
const response = graphviz.layout(dotQuery, 'json', 'dot');

Any insights/help/fix would be very welcome!

@GordonSmith
Copy link
Member

Thanks for the detailed report - looks like a re-entrant issue with the GraphViz c++ library (as its typically used as a CLI they may not care too much, but I will open an issue there anyway).
I have two workarounds:

  1. The issue is happening in a free memory call (the error is a memory freed twice) and commenting out that call "fixes" the issue at the expense of leaking some memory
  2. Add an "unload" method similar to the "load" method, which clears out the current instance.

@MikeaMihali123
Copy link

Thank you for the quick response. I am a teammate of @laurajunco working on this topic.
For the proposed workarounds:

  1. Would the leak stack up quickly ? as our use case has multiple calls for layout triggered by user interactions
  2. For this second workaround ,we kinda did something similar as a fix by running the graphviz code inside a Worker and destroy/creating the Worker each time. This fixes the issue but it causes a significantly slower response time to get the layout 😞

@GordonSmith
Copy link
Member

The workaround would be faster than creating worker instances each time - and I am not seeing a wall clock difference with or without it (on a loop of 50 calls).

Note: Running in full Debug mode did throw out some interesting logging and I noticed for your example if your remove the weights for the edges that run between the clusters that your issue goes away for my test of 50 calls on a single load...

I suspect if you reduce your edge weight to smaller numbers (1->5?) that it will also help with performance?

All going well I will push a release today (I used this issue to refactor all the c++ debugging generation code to simplify between release an debug with breakpoints now working in c++!!!) - but I suspect with the suggested tweaks above you may not need it now...

@GordonSmith
Copy link
Member

I have released a version with the unload method see: https://github.com/hpcc-systems/hpcc-js-wasm/blob/trunk/src/__tests__/graphviz.ts#L165-L179

I have also opened a ticket GraphViz for this specific issue: https://gitlab.com/graphviz/graphviz/-/issues/2331

@laurajunco
Copy link
Author

Thank you @GordonSmith! I'll test it out and let you know :)

@laurajunco
Copy link
Author

Hey, @GordonSmith just to let you know that the fix worked perfectly for the issue. Thanks a lot for the quick support ✨

@chouzz
Copy link

chouzz commented Jun 29, 2023

Hi @GordonSmith Seems I met this problem again, and maybe it's the same problem, but I found this still not solved in the latest hpcc-js-wasm version, I reproduce this problem hrere:

https://codesandbox.io/embed/graphviz-dependency-tree-forked-85y2c9?fontsize=14&hidenavigation=1&theme=dark

The origin issue comes when I used d3-graphviz, we discussed this problem and found seems it's related hpcc-js-wasm and graphviz: magjac/d3-graphviz#280

@GordonSmith
Copy link
Member

@chouzz I replied on the other thread, but your missing a space before the second submodule...

@chouzz
Copy link

chouzz commented Jul 3, 2023

Thanks replay, but the space is not a problem.

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

4 participants