Alexey-T / CudaText

Cross-platform text editor, written in Free Pascal
Mozilla Public License 2.0
2.4k stars 167 forks source link

Severe memory leak with LSP plugin usage #5407

Closed DUOLabs333 closed 1 month ago

DUOLabs333 commented 4 months ago

To be fair, this could be due to have large files open (I have 2 3MB files open), or my LSP (is there a way to disable LSP and syntax highlighting for large files?), but after ~5 hours, if I keep CudaText open, the process takes up 3GB of memory.

EDIT: It is not due to the LSP --- memory is creeping upwards, but none of the LSP servers are running.

Alexey-T commented 4 months ago

we need to know it is not lsp. so remove plugin dir (py/cuda_lsp) and try without it.

i cannot repeat it yet. @veksha do you see this issue on big files with or w/o lsp?

Alexey-T commented 4 months ago

(is there a way to disable LSP and syntax highlighting for large files?

yes, we have option "ui_max_size_lexer". set to 2 to disable highlight for >2mb files.

veksha commented 4 months ago

@DUOLabs333 what file type? (lexer). LSP is disabled and still issue reproduces?

DUOLabs333 commented 4 months ago

@veksha It takes a while to see if a change has an effect, so I'll have to wait a few hours.

DUOLabs333 commented 4 months ago

UPDATE: Yes, if I disable LSP, there is no memory leak.

Alexey-T commented 4 months ago

so it is not Cud's issue, okay.

DUOLabs333 commented 4 months ago

I mean, it still is --- the memory is attributed to the CudaText process, not some external LSP process.

Alexey-T commented 4 months ago

this memory is allocated by the LSP plugin, ie plugin issue...

DUOLabs333 commented 4 months ago

I was going to attach a memory profiler to the LSP plugin, but since it doesn't have an entrypoint, I couldn't actually put it anywhere. Was the release version of CudaText compiled with debug symbols (i.e., fpc -g), or do I have to recompile it myself?

Alexey-T commented 4 months ago

Was the release version of CudaText compiled with debug symbols (i.e., fpc -g), or do I have to recompile it myself?

app had debug symbols but in 'external file' .dbg. I don't keep this .dbg file so better recompile the app. in the project options, you have option 'use external debug symbols file'

Screenshot from 2024-03-02 10-09-12

veksha commented 4 months ago

I was going to attach a memory profiler to the LSP plugin, but since it doesn't have an entrypoint, I couldn't actually put it anywhere.

What profile do you use? Entry point is a beginning of an init py file.

DUOLabs333 commented 4 months ago

That won't work --- memray uses a context manager, so as soon as the import is done, the profiler with finish, which is not what we want.

DUOLabs333 commented 4 months ago

Can you make a debug version of CudaText (arm64+qt5) with embedded debug symbols? I can't compile CudaText, and I have no idea why.

Alexey-T commented 4 months ago

here is the file 'cudatext', 53Mb file:

cudatext-arm64qt.zip

DUOLabs333 commented 4 months ago

Yeah, so I ran it through heaptrack, and the biggest leaker was in QImageData (which might be something to look into), but the second biggest leaker was within Python (which I'm assuming correlates with the plugins). I'll run it longer to see exactly which plugin it is.

EDIT: Though, if it turns out that it is QImageData that is the leaker, it may be because you are generating a lot of the small red boxes with question marks in the middle without destroying them once you're done with them (which is why it only shows up when using LSP)?

DUOLabs333 commented 3 months ago

Ok, what I found interesting was that even though cudatext went from 280MB to 700MB, heaptrack says that only 44MB was consumed.

Alexey-T commented 3 months ago

I tried to see that imagelist-loading is redundant. i added 'print' to cuda_lsp/language.py near

        _ind = imagelist_proc(_h_im, IMAGELIST_ADD, value=icon_path)

but as I see 'print' shows only once per each file. I switched 2 python files and edited them with pylsp: only one 'print' per file anyway.

Alexey-T commented 3 months ago

I found 2 leaks in atsynedit core: imagelist for 'popup menu for folding-gutter-column' and imagelist for 'gutter decors'.

Is it better? beta for arm64-qt5

cudatext-arm64qt5_2.zip

imagelist for 'bookmarks icons' was OK already coz it is global for all editors. I wrote now to the wiki about BOOKMARK_SETUP:

DUOLabs333 commented 3 months ago

No, the leak is still there.

Alexey-T commented 3 months ago

the biggest leaker was in QImageData

is it improved now?

DUOLabs333 commented 3 months ago

Interestingly, yes --- however, I definitively pinpointed the leak to the LSP plugin (if I prevent the server from starting, the memory usage remains constant).

Alexey-T commented 3 months ago

Interestingly, yes

Ops, I see now that my 'fix of memleak' is not correct, it must work the same as before (ie I think that 2 imagelists were auto-freed even before the fix)... so QImageData is not leaked now? strange.

DUOLabs333 commented 3 months ago

However, take it with a grain of salt, as what heaptrack reports does not necessarily match reality. Therefore, it's possible that QImageData still leaks.

DUOLabs333 commented 3 months ago

More concretely, the number of objects that are not freed is increasing, which is definitely a problem.

DUOLabs333 commented 3 months ago

I think I found the culprit: 2 instances of class cell are made regularly. Where is the cell class used?

Alexey-T commented 3 months ago

cuda_lsp code does not have cell type. Cud core cannot have it too - pascal types names start with a T.

DUOLabs333 commented 3 months ago

Oh, never mind --- it's a native Python datatype. But it is interesting that 2 new ones are created regularly.

Alexey-T commented 3 months ago

('cells' are not used by threads, I removed prev post).

DUOLabs333 commented 3 months ago

And as it turns out, I made a mistake in my profiling --- I was saving gc.get_objects() to a global variable, which means that all objects in it could not get collected (as the list incremented each of their reference counts). This of course showed up as a monotonically increasing object count. By only saving the classes, I got a more accurate picture --- it seems that there's no memory leak in the number of Python objects created (of course, it's possible that the leak is in the size of an object, like strings).

DUOLabs333 commented 3 months ago

I think I have a way to use a proper memory profiler --- when cudatext instantiates a Command object, is it used for the entire duration of the cudatext program, or does it make new ones throughout the program's lifetime?

Alexey-T commented 3 months ago

when cudatext instantiates a Command object, is it used for the entire duration of the cudatext program

yes, it is.

DUOLabs333 commented 3 months ago

Ok, after profiling for a while, I found that the biggest memory usage came from process_queues.

veksha commented 3 months ago

Ok, after profiling for a while, I found that the biggest memory usage came from process_queues.

You can comment out some lines there and repeat process. 🙂

DUOLabs333 commented 3 months ago

As it turns out, queue.get doesn't necessarily clear memory.

DUOLabs333 commented 3 months ago

You can comment out some lines there and repeat process.

What do you mean?

veksha commented 3 months ago

if you found the line - nevermind.

DUOLabs333 commented 3 months ago

As a last resort, @Alexey-T can you recompile cudatext but with -gv enabled? I want to see if the leak is in one of the Pascal-native functions using valgrind.

Alexey-T commented 3 months ago

no problem, it's here: http://uvviewsoft.com/c/

DUOLabs333 commented 3 months ago

I was able to prevent (or at least severely limit) the leak by disabling the usage of the DocBook class. However, since that is critical to the plugin (without it, nothing works), it is not a proper solution. This does mean though that the problem lies in that class.

DUOLabs333 commented 3 months ago

I figured it out --- the leak is in cudatext_api.timer_proc. Where is the source code for this function?

veksha commented 3 months ago

I figured it out --- the leak is in cudatext_api.timer_proc. Where is the source code for this function?

Maybe timer calls python code? And python garbage collection is postponed, idk.

Alexey-T commented 3 months ago

I figured it out --- the leak is in cudatext_api.timer_proc. Where is the source code for this function?

in pascal, in CudaText.

DUOLabs333 commented 3 months ago

I'll try to fix it, but maybe we can move the timer implementation to pure Python? I'm not sure why it's in Pascal to begin with.

DUOLabs333 commented 3 months ago

Yeah, I just rewrote the timer mechanism in Python, and the leak doesn't occur. Looking at api_timer_proc, I'm 90% sure it has something to do with garbage collection (Obj may not be properly freed).

DUOLabs333 commented 3 months ago

Should I open a PR (the patch only modifies cudatext.py)?

Alexey-T commented 3 months ago

I am totally unsure that Py timer implementation will work in all plugins as previously. we may get regressiions easily. don't do PR, yet, but show the code pls.

Alexey-T commented 3 months ago

I'm 90% sure it has something to do with garbage collection (Obj may not be properly freed).

it is freed on the pascal level, right? then how it can be wrong? it is usual deletion of pascal object... if you have idea how to patch pascal code here, pls show.

DUOLabs333 commented 3 months ago

Yeah, I'm noticing that the LSP plugin does not respond as quickly as before to hover requests with my implementation. It may have something to do with thread starvation.

DUOLabs333 commented 3 months ago

it is freed on the pascal level, right? then how it can be wrong

It don't know much about Pascal (which is part of the reason I'm rewrote the timer in Python), but just because it is freed in Pascal does not mean the underlying resources are freed in Python.

Alexey-T commented 3 months ago

just because it is freed in Pascal does not mean the underlying resources are freed in Python.

pascal TTimer object don't have assosiated Py resources, AFAIK.