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

Question: Is there memory leak in jsPanel4? #121

Open
johacode opened this issue Apr 3, 2020 · 20 comments
Open

Question: Is there memory leak in jsPanel4? #121

johacode opened this issue Apr 3, 2020 · 20 comments
Labels
info information only question

Comments

@johacode
Copy link

johacode commented Apr 3, 2020

First of all your jsPanel library is very good library! Thanks for it.

I am just started to learn memory leak studies. My question is there leaking memory when one jsPanle4 dialog at time is opened and closed two or more times?

For me it looks like there is memory leak.

I am using Memory pane of Google Chrome 80.0.3987.163 (64-bit) in Windows 10 for my studies.

jsPanel version I am using is
CSS: https://cdn.jsdelivr.net/npm/jspanel4@4.10.0/dist/jspanel.css
script: https://cdn.jsdelivr.net/npm/jspanel4@4.10.0/dist/jspanel.js

Steps to reproduce

  1. open https://jsfiddle.net/johacode/cj9d1syb/6/ page with Google Chrome
  2. press Run button in top row of page
  3. open developer tools of Chrome
  4. select memory pane of developer tools
  5. select 'heap snapshot' radio button in memory pane
  6. Select fiddle.jshell.net as 'Javascript VM instance' and press 'Take snapshot' in bottom of memory pain to create 'snapshot 1'
  7. press open button in 'jsPanel test' pane and close the opened jsPanel
  8. press 'take head snapshot' in memory pane (record button in header of memory pane, snapshot 2 created)
  9. press second time open button in jsPanel test pane and close the opened jsPanel
  10. press 'take head snapshot' in memory pane (snapshot 3 created)
  11. Now select 'Snapshot 3' from heap snapshot list of memory pane
  12. check that perspective is summary and write word 'detached' without quotes in class filter box of memory pane (second row from top)
  13. see that there is several different detached elements in memory which can not be garbage collected because of existing javascript reference

These detached elements are not found after 1st open and close round but they appear after 2nd round and they continue to increase after that. Total increase is approximately 10kB in every following round.

For me it looks like those elements are not garbage collected because global 'elmt' variable does have reference to them. Screen capture from one detached element

This is the point I have manged to study this issue so far.

@Flyer53
Copy link
Owner

Flyer53 commented Apr 3, 2020

@johacode
Thanks for the praise 😄

You might be correct, please see #99
But I ask you to consider that I'm not a professional programmer. It's more or less a hobby to keep my brain working 😏 So memory leaks are not a topic I have a really good idea of and currently I don't know what to to about it.

I didn't look at your fiddle yet. But will do ... and thanks a lot for your effort!

@Flyer53
Copy link
Owner

Flyer53 commented Apr 4, 2020

@johacode
Ok, I took a first look abd it seem to originate from the dragit/resizeit interactions. If you create the panel with dragit/resizeit set to false everything seems fine. Maybe you want to check that as well to confirm my result.

@johacode
Copy link
Author

johacode commented Apr 6, 2020

Thanks for fast response. Yes, it looks like there is not seen detached elements anymore if panel is created with resizeit and dragit disabled.

      panelHdl = jsPanel.create({
        ...
        resizeit: false,
        dragit: false
      });

@johacode
Copy link
Author

johacode commented Apr 6, 2020

After some more investigation my best guess is that event listeners added in rows 2213 and 3332 are preventing garbage collection after jsPanel is removed from DOM.

2212: jsPanel.pointerup.forEach(function (item) {
2213:  document.addEventListener(item, function (e) {

3331: jsPanel.pointerup.forEach(function (item) {
3332:  document.addEventListener(item, function (e) {

@Flyer53
Copy link
Owner

Flyer53 commented Apr 6, 2020

You're right. I could "fix" it already for the dragit interaction. Resizable seems more complicated though ... working on it ...

@Flyer53
Copy link
Owner

Flyer53 commented Apr 6, 2020

Well, the first fix worked only in FF, not in other browsers.
I think I'm on it, but I won't get it done in a few hours ...

@xnopasaranx
Copy link

Hello there! I just started using you library this week and am quite impressed. Fixing memory leaks however is not fun. What's the status on this? Did you ever get around to revise the code in regards to this? I am going to integrate your lib for a demo project anyway, but would like to use it in the future and maybe I can help.

@Flyer53
Copy link
Owner

Flyer53 commented Jun 19, 2020

@xnopasaranx
Hi there and thanks for the praise 😄
Well, as far as I can tell the memory leaks originate in the code of the dragit and resizeit interactions. At least @johacode, who opended this issue, confirmes having no issue when creating a panel with dragit and resizeit disabled. But to be frank, I didn't do anything about it yet. The topic Memory Leaks is not that easy to grasp for me anyhow as amateur programmer. On the other hand nobody ever reported having any "real" problem with it so far. These are no excuses of course ....
My approach would rather be to "redesign" the dragit and resizeit interactions in a way that would make them more modular. So that a base functionality is provided by default that can be extended by "plugins" you only load if you really need them. But so far that's nothing more than an idea ...
However, if you have the time and motivation to look into my (probably much to complicated) code I would appriciate that a lot.
The global object jsPanel has the two methods jsPanel.dragit() and jsPanel.resizeit() which are called by the individual panel when it's created. I wonder if it would already do the trick if I moved those two methods to the individual panel in its entirety?

Regards,
Stefan

@Flyer53
Copy link
Owner

Flyer53 commented Jun 21, 2020

@johacode
I created a version where the code for the dragit and resizeit interactions is moved from the global object jsPanel to the individual panel. My impression is that it improves this issue. Maybe you want to give it a try. Feedback would be appriciated ...
jspanel-v4.11.0-beta.2

@johacode
Copy link
Author

I am using Chrome 83.0.4103.106 at the moment. Your changes seems to reduce the leakage a bit.

It looks like jspanel-v4.11.0-beta.2 does reduces occurance of 'detatched EventListeners and V8EventListeners' from 25 to 20 .

I did checked that EventListeners that are left in memory are referring to following places

jsPanel.js:4496 (in self.sizeit function)
jsPanel.js:3678 (in self.drag function)

@Flyer53
Copy link
Owner

Flyer53 commented Jun 22, 2020

@johacode
Thanks a lot for your effort.
Well, those two references point to the pointerup handlers of the dragit/resizeit interactions which are attached to the document and don't get removed when the panel is closed. Sounds easy to fix ... but isn't.
The code of the dragit/resizeit interactions with all their options is the most complicated of the whole script (at least for me) and I simply didn't know about something like memory leaks when I started to code my own drag/resize methods. So for now it's something we have to live with until I start to rebuild dragit/resizeit from scratch.

@Flyer53
Copy link
Owner

Flyer53 commented Dec 9, 2020

@johacode @xnopasaranx
I just released v4.11.2 with updated dragit interaction code reducing (not eliminating) memory leaks caused by jsPanel.

@Flyer53
Copy link
Owner

Flyer53 commented Dec 10, 2020

@johacode @xnopasaranx
I made another change that seems to further improve the memory leak issue caused by jsPanel.

To give it a try download alpha.jspanel.de/downloads/jspanel-4.11.3-alpha.zip

I would highly appreciate some feedback ... 😏

@johacode
Copy link
Author

Sorry that my response take couple of day but I have been busy with other things.

Current version of Chrome I am using is 87.0.4280.88 (64 bit).

I checked how original test case works that Chrome browser with the versions 4.10.0 (original), 4.11.2 and 4.11.3-alpha of jsPanel

Here is new picture containing memory pains of Chrome. Notice that picture is such wide that all it shown with scrollbar on bottom.

Screen capture from all detached elements with three jsPanel version

It looks like 4.11.2 have least detached elements.

Here is one blog post I found useful.

description for shallow and retained sizes

There I found these descriptions

Shallow size
This is the size of memory that is held by the object itself.

Retained size
This is the size of memory that is freed once the object itself is deleted along with its dependent objects that were made unreachable from GC roots.

@Flyer53
Copy link
Owner

Flyer53 commented Feb 20, 2021

With v4.11.3 I could improve this issue a little bit more. But that's probably as good as I can make it until I completely recode the dragit/resize code ...

@Flyer53 Flyer53 added the info information only label Mar 9, 2021
@bmbabcock
Copy link

Hey @Flyer53, great project! I've forked the repo and implemented a fix for these memory leaks by using an abort signal to detach the events on the document and window when the panel is closed. I was hoping to test it out and send a pull request (assuming it works). I can't figure out how to build the project. I assume I am missing something obvious, could you give me some advice on how to build it?

@Flyer53
Copy link
Owner

Flyer53 commented Aug 7, 2023

Hi @bmbabcock ,

Sorry for the late answer. Couldn't you just modify the source file and mail it to me? Then I'll check it out. Although I know about these memory leaks, so far nobody reported any real problem with that and I don't consider it as a real problem. Did you observe any real problem?

Info:
As far as I can judge it now jsPanel v4.xx is at its end of its developement unless some severe issue comes up. And I don't know yet whether I'll ever start a v5.

@bmbabcock
Copy link

jspanel.js.txt

Hey @Flyer53,

I've sent an email to the info address on your site and also attached a copy here. We use the library in a large application and a large number of panels are opened and closed while the application is in use. These panels contain forms some of which are quite extensive. This results in large chunks of detached HTML being retained in the heap.

I expect our usage isn't typical but we see significant improvements with the change. I can't give you exact numbers because part of the panels were also retained by a second part of our code base. With the full panels now being garbage collected we have seen memory improvements from 1.5-2gb to ~0.5GB.

@Flyer53
Copy link
Owner

Flyer53 commented Aug 10, 2023

@bmbabcock
Hey Byron, thanks a lot for your work, the explanation and the modified file 👍🙂
Currently I'm on vacation for the next two weeks at least. So it will take some time until I take a real look at it.

Almost all jsPanel use cases I know of are in some kind of backend or otherwise access restricted. So, to be frank, I don't know very much about how it's actually used. And feedback is quite sparse until a problem comes up.

@Flyer53
Copy link
Owner

Flyer53 commented Oct 14, 2023

@johacode @xnopasaranx
I created a preliminary v4.17.0 with some code contributed by @bmbabcock in order to minimize the known memory leaks resulting from the resizeit/dragit interactions. Please download the files and check it out. Feedback highly appriciated 😏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info information only question
Projects
None yet
Development

No branches or pull requests

4 participants