-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Probably false alarm regarding small Server Rendering Memory leak #501
Comments
This has already been done in the https://github.com/shakacode/react-webpack-rails-tutorial and some of the docs are updated. Possibly, this is complete. |
Hey, I changed therubyracer to mini_racer just like here, but instead of a speedup I got a 2-3x slowdown. Before:
After:
Any thoughts on what could be wrong? Must I somewhere specify that I want to use mini_racer or is it just enough to add it to the Gemfile? |
@mariusandra Please investigate this, as it's not what we experienced with https://www.friendsandguests.com/. I suggest you do some more detailed profiling in production. Do you use new relic? |
@justin808 I investigated and found the bug! I was stuck with execjs 2.6, but needed 2.7. Upgrading it solved the situation. I'm not sure what execjs 2.6 was using for the big slowdown, probably an external node that was reloading the big server bundle every time. I do see a speed increase now! With therubyracer:
With the mystery node:
With mini_racer:
New relic shows that the average time spent in the view that renders the react component (for this page) went from 57.6ms (to 956ms) to 38ms, a 35% improvement. So, thanks a lot and I hope this helps someone with a similar issue in the future. |
Hey! Somehow I got a new issue. The blue part in this graph shows how long the react component renders on the server side: It starts out great, but then keeps getting longer and longer until something either kills it or something gets garbage collected. As a temporary (Windows 98 style) fix, I added a cron job that restarts the server every 30min, stabilising the situation., but obviously this is not ideal :). @justin808, have you experienced something similar? Any hints on how to debug this? |
@mariusandra there's a lot of good info on debugging react performance here: http://benchling.engineering/performance-engineering-with-react/ |
@mariusandra No idea on how to fix what you're seeing. |
I finally fixed my memory leak issue. It came from using a component called They recommend calling Debugging this memory leak took a bit of time. In case anyone else needs to do this, here are the steps I took:
Now you can see if the memory is actually leaking (keeps growing after every Mark-sweep) or gets cleaned up (resets back to a stable amount after every Mark-sweep). If you see stuff like this, you have a memory leak:
However this is probably normal:
7.1) First one: The above script will give you several files Open up Chrome, go to the developer tools and select "Profile". Click "Load" and open two of the files. E.g. "heap-100" and "heap-300". Then click into one of them and compare with the other. You should see something like this: The bigger the distance between the files (100 to 300 vs 100 to 2100), the better. If you see a lot of duplicate objects that are a multiple of the distance (200 in this case), you probably have a leak there. If you see any React components hanging around (or 100 of them), you definitely have a leak. Figure out how to get rid of it. This article can be of help. 7.2) Just start removing elements from your React components and re-run the test. Keep doing this to narrow down the line that leaks. For me the leaks stopped once I removed the line for Throughout this, I found that react_on_rails itself has a small memory leak. I'm not sure if it's something that only happens in development or what exactly causes it, but apparently all the console messages, props and railsContexts are kept between server renders: This was true for my application (Apprentus) and for In any case finding and fixing the I'll post again if/when I find something, however feel free to take a crack at this. (Yes, you who are reading this now! :)) Cheers! |
FYI the README mentions this issue still |
@dzirtusss @robwise @dylangrafmyre Heads up. We need to fix this. |
We're keeping the console messages in the context. We need to clear the console logs at the end of each rendering. We seem to be doing that:
If we need a fix for this.
Code References:
|
@mariusandra @malonehedges Any idea if we might have a memory leak? I don't think so from looking at the code. |
One potential issue is that if a JS library has a memory leak, then server rendering will magnify this leak. We might want an option to delete the context after so X renders. |
@justin808 Hey, I'm not sure. The code indeed looks like it clears the messages, but those lines were also there when I wrote my long comment in December... and they didn't seem to back then. I don't know enough about V8 to guess how it stores and discards references to objects between calls that reuse contexts. Someone with more knowledge of the matter should give their opinion here :). One option to try is "method 3" from this page: https://appendto.com/2016/02/empty-array-javascript/ Basically instead of
to do
I tried checking if it would make a difference, but my react_on_rails + react-webpack-rails-tutorial builds are all messed up. I can't promise when I'll find time to fix them and test again. So someone else can take over. However even with this fixed, the issue with a memory leak in a JS library still remains. |
https://github.com/shakacode/react_on_rails/search?utf8=%E2%9C%93&q=therubyracer
mini_racer is faster!
The text was updated successfully, but these errors were encountered: