jcubic / jquery.terminal

jQuery Terminal Emulator - JavaScript library for creating web-based terminals with custom commands
https://terminal.jcubic.pl
MIT License
3.11k stars 570 forks source link

Increase Consumption of Memory Using Too Many Echo() #748

Open KiddoV opened 2 years ago

KiddoV commented 2 years ago

I have question related to jQuery Terminal

I made an application that will has like 8 terminal instances open at the same time. While using the for loop to call .echo() on each terminal, the memory just keep building up (According to the Windows Task Manager) the msedgewebview2.exe is going from 90MB to 120MB and more and more... Is there a way that I can fix this issue? I don't think JQ Terminal instance will save all of my echo output right?

jcubic commented 2 years ago

This is not memory leaks, basically, almost any library or project, if you keep Adding DOM nodes it will make the memory grow. THis is how websites work.

Just try adding items to the list in a loop and you will see how memory is growing.

You can do two things limit the number of things on the screen with outputLimit since you probably don't need all those DOM nodes on the page. The other fix that could fix the way browser work is tracked in issue Add/Remove DOM nodes dynamically to show only what's visible on terminal (but I don't know if it will ever get implanted).

KiddoV commented 2 years ago

@jcubic Thanks for the reply. I already tried to set outputLimit to 250 on each terminal. It helps a little bit, but memory is still growing slowly. I hope it will stop growing at some point if outputLimit is reach or the app will crash if user keep using it unless they reload the app. If this is not the JQTerminal problem, I guess I have to figure out the solution myself. Thanks for the tip anyway!

jcubic commented 2 years ago

there is one thing that may cause memory growth, which is cache. To speed up rendering there is a cache that saves all output in Map I can maybe create some invalidation of that cache (after some amount of time) or use WeakMap for this.

It seems there is also an option useCache you can try to disable the cache.

KiddoV commented 2 years ago

@jcubic thank you. I will try that.

Is there a method that I can use to clear all caches or anything to make all the terminals fresh like first time it load again?

jcubic commented 2 years ago

There is clear_cache method, I think that It's not documented.

KiddoV commented 2 years ago

@jcubic I tried useCache: false, and also call clear_cache. It just help a little bit, but memory is still increasing overtime.

Also when using too many echo(), my app seem to not responsive when resizing and min/maximizing anymore, and all of the terminal scrollbars just visible even though they are not in focus.

jcubic commented 2 years ago

I've added milestone 2.33.0, I don't use them much, but this will make me prioritize the issue. Since I think it's critical. Memory leaks are bad.

jcubic commented 2 years ago

There are two issues:

Making all lines gone after they out of view by the output limit will be a breaking change, but I can make an option to toggle between two behaviors.

I've tested using:

    var term = $('body').terminal({
        clear() {
            term.clear_cache().clear();
        }
    }, {
        greetings: false,
        clear: false
    });

adding some lines to terminal and then calling clear seems to clear memory, so I think that the problem are lines that are always available.

KiddoV commented 2 years ago

I had my app running everyday to test the memory. It has 8 terminals. I made a function to call echo every 100 milisecs, and was using term.clear_cache().clear(); to clear everything after 1000 lines. It helps, but memory still slowly increasing like really slow, but still... (Like ~50MB for a week). I will try your new version again and let you know how it go. Thanks!

KiddoV commented 2 years ago

So I am on v2.33.2 Jquery Terminal. I make an app to test the performance. The app has 8 terminals running simultaneously. I call echo() for every second on all 8 of them. I use term.clear_cache().clear() to clear every 1000th line. It have been running 24/7 from May 23, 2022 until now June 15, 2022. The memory when I started the app at 96MB, now it is at ~600MB. So it is still increasing over time. I run the app on webview2 engine, so the memory goes up on the msedgewebview2.exe process.

Capture1

Capture2

jcubic commented 2 years ago

Thanks for the update. Unfortunately, I have no idea what causing the increase in memory, I also don't know how to find what is causing this.

It's definitely not a cache, but to be sure I can disable all the cache temporarily and test. If it's not the cache then I will go through the whole code and look for closures maybe there will be some visible error that saves the closure for later.

jcubic commented 2 years ago

Can you maybe test in a real browser to ensure it's not a bug in WebView? Chrome browsers have a memory profiler that can find the issue with memory.

KiddoV commented 2 years ago

I will try on Chrome. Thanks!

jcubic commented 2 years ago

Do you have this app somewhere online? If not can you publish? I will test it myself.

jcubic commented 2 years ago

For future reference: Troubleshooting a JavaScript memory leak

KiddoV commented 2 years ago

Do you have this app somewhere online? If not can you publish? I will test it myself.

Best way to test my app is using the exe itself. (~8MB) Here is the bin file of my app, I allowed the debugging console so you can look more deep into it. Or even right click to open "Inspection" mode for front-end. This app required Webview2 Runtime, most Windows 10 already had it built-in. Google drive link to my app

On the biggest terminal at the bottom, type xdots Enter to enter the interpreter > then run debug testmemoryonecho <number of time you want the 8 terminal above to print> for example run debug testmemoryonecho 10000. Then leave it sometime and you will see the memory is increasing.

If somehow Google blocked my file, please use this link: (This is one-time download link, file will be delete after download!!!) https://file.io/leQ6tWdJBKwk

Thanks,

jcubic commented 2 years ago

I don't use Windows. And on Browserstack I can only test websites in the browser.

Do you have a working example that shows memory leaks in actual browser? If it works in Google Chrome it means that Windows Webview is broken and you should report the issue to Microsoft (not sure it will help in any way since M$ sux at support).

KiddoV commented 2 years ago

In that case, I need time to re-create a lite version of it to make it works on stand-alone browser like Chrome. I will keep you update about that.

jcubic commented 2 years ago

@KiddoV were you able to find anything? Maybe something I can test?

KiddoV commented 2 years ago

Sorry for the super late update! I made a lite version of my app to port it on Chrome. Looks like the memory still increase overtime, but it is x2 slower comparing to the webview2. I can give you the sample if you want to try.

jcubic commented 2 years ago

Sure, I will check with the profiler maybe I will find something.

KiddoV commented 2 years ago

Will send it tomorrow when I get back to work.

KiddoV commented 2 years ago

@jcubic https://filetransfer.io/data-package/RNp812VA#link Here you go!

jcubic commented 2 years ago

I can try to replace Map with WeakMap everywhere in the code. It has similar support. And when string will not be used it will be garbage collected.

jcubic commented 2 years ago

I think that it fixed the issue, I started with ~700MB of memory for your application then it grow to about 900MB but after some amount of time, it decreased to ~700MB again so it probably garbage collected everything including DOM nodes.

jcubic commented 2 years ago

I would need to delete the API clear_cache since it will not be needed anymore.

KiddoV commented 2 years ago

Sounds good. Glad you got it fixed!

jcubic commented 2 years ago

I was testing again and got errors from the library. It turns out that you can't use strings as keys in WeakMap. I'm not sure how it was working when testing your app.

jcubic commented 2 years ago

I have another idea, the initial reason for the cache was that processing was slow when there was a processing of the same complex string over and over like when you have a big chunk of syntax highlighted text as a command and you navigate over the command. It was slow to process everything on each keypress. That's why the cache was added.

So I can just store one value in the cache and see if it will not reduce performance and remove memory consumption. Your demo is special because you echo different text each time in a loop. So the cache is useless in your case and it only consumes memory.

When ready, you will be able to test your app, and we will see if that works.

KiddoV commented 2 years ago

So... what is the solution in your mind, programmatically?

jcubic commented 2 years ago

Maybe replace Map with a custom class that only keeps one value and if the value changes old one is discarded. This way I will not need to modify the code much.

jcubic commented 9 months ago

Just realized that I should probably add a timer to cached values. So they are removed from cache and probably timeout option.

jcubic commented 5 days ago

Just found this tool that can find memory leaks in web apps:

https://github.com/nolanlawson/fuite

Maybe it can be used to find what is the root cause of memory leak.

KiddoV commented 5 days ago

@jcubic Thanks, I'll give it a try.