zarunbal / LogExpert

Windows tail program and log file analyzer.
Other
946 stars 162 forks source link

memory leak #20

Open yoramo1 opened 6 years ago

yoramo1 commented 6 years ago

I'm a big fan of this application. but i encountered a very annoying bug. using version 1.6.6. if I leave LogExpert open for a log time it increases the memory size used and eventually will make the computer work very slow.

thanks

Hirogen commented 6 years ago

this is a Bug I encountered in all Versions

Can be easily retested with a continous logfile that is written while it is viewed in logexpert.

Logexpert will eventually use all the computers memory (in my case 15gigs+). After all the memory is used, the computer will be unresponsive.

Workaround is to restart it regularly or don't let it run overnight!

zarunbal commented 6 years ago

Thanks for the issue. I'll try to reproduce this problem.

As far as I understand the application grows if a line is appended?

yoramo1 commented 6 years ago

I do not have a reproducible scenario for this problem.

it looks related to appending lines to the log file.

after the problem LogExpert can not be closed you need to kill it from the task manager.

1inirt1 commented 6 years ago

I totally agree that LogExpert is awesome and I love using it. Except for this one memory leak issue. We had a test system crater over the weekend because someone left LogExpert open to a rolling log file that gets updated at least once every 15 seconds.

zarunbal, this bug is pretty easy to replicate so if you'd like exact steps to replicate please let me know because the sooner this bug can be resolved the sooner my colleagues and I can replace a whole bunch of different log viewer programs with LogExpert once and for all. Thanks and keep up the great work.

zarunbal commented 6 years ago

I can reproduce this bug, and already added some improvements, which lowers the increase of memory per time / line. But to find / fix the root of the problem this is still some work (despite that the problem is reproducible).

There would be a possibility for a workaround of this memory problem. I could create a 32 bit only application (it would still work on a 64 bit machine but could only address around 2 GB of ram). If the application reach the limit it will crash. This would be a rough workaround but it would prevent that LogExpert uses all the memory.

@1inirt1 thanks for your feedback. I understand your problem and the related problems for your. I work at this bug as hard as I can but this is only in my freetime.

fremag commented 6 years ago

With dotMemory (profiler) I looked at this issue. I ran LogExpert on a file that was updated 20 times /secondes. After a few hours, the memory used was around 600 Mo. I think it should not be that high. There are some buffers for each file watched but it's limited to 10 x 100 lines. Even if a line is around 1 ko, the corresponding memory is around 1 Mo, not 100 Mo.

I compared some dumps at the beginning and end and noticed a lot of objects related to the DataGridView.

image

(my file was around 1 Mio lines)

The main DataGridView uses VirtualMode so only a few DataGridViewrow should be created (the one displayed + some waiting for GC) but not 25% of the RowCount.

(to be continued)

fremag commented 6 years ago

So I tested with a basic project: one form with one datagridview in virtual mode and 1Mio rows (set with RowCount property). I dumped the memory and noticed only a dozen of DataGridViewRow instances.

So VirtualMode is ok. I restarted LogExpert with my large file and dumped memory: loading the file tooks a few seconds but there was only a small number of DataGridViewRow.

Then I modified my small project to add this line:

            dataGridView1.CellFormatting += (sender, args) =>
            {
                args.CellStyle.BackColor = dataGridView1.Rows[args.RowIndex].Selected ? Color.Gray : Color.White;
            };

I scrolled up and down, then dumped memory I saw a LOT of DataGridViewRow instances.

I did the same thing with LogExpert: load the big file again and scroll up/down => huge number of DataGridViewRow.

I think the call to dataGridView1.Rows[args.RowIndex].Selected CREATES an instance of DataGridViewRow and is linked to the DataGridView object and never garbage collected.

Loading a big file is not an issue but scrolling is and the tail mode scrolls a lot so the memory used over time increases a lot.

I hope I'm clear and I didn't say any stupid things, any comment is welcome. I don't have a solution yet for LogExpert.

hraab commented 6 years ago

I remember that there are some conditions in which the DataGridView doesn't dispose the cells/lines even when in virtual mode. For example when lines are selected. It is documented anywhere in the MSDN.

Could be that some code in LogExpert prevents the DataGridView from re-using/disposing lines.

Could you test with highlighting completely disabled and using no columnizer? Also consider to use a debug build and switch off the word highlighting mode (Debug menu). This enables a different cell renderer (older one).

hraab commented 6 years ago

OK, I just read your comment @fremag. This completely makes sense. LogExpert selects the last line when tailing. So I guess this leads to the problem that the DataGridView does not dispose the cells.

fremag commented 6 years ago

Here: https://github.com/zarunbal/LogExpert/blob/57343b9d5d840fa3bddb298e536ddefb184dc597/src/LogExpert/Controls/LogWindow/LogWindowPrivate.cs#L700

Or here: https://github.com/zarunbal/LogExpert/blob/6f6cd5f183f18040f1d04e512cb6cc7233363e1c/src/LogExpert/Controls/LogWindow/LogWindowsPublic.cs#L912

fremag commented 6 years ago

Is it possible to call dataGridView.Rows.Clear() sometimes ?

fremag commented 6 years ago

Too late to test this, I need some sleep but tomorrow is another day. Stay tuned :)

fremag commented 6 years ago

I found two articles about virtual mode:

A row is "shared" (ie one DataGridViewRow object is used to display many virtual rows) until you access to it (Selected property for instance). Then it's cloned and remains in memory (forever ? until you dispose the grid ?)

There is an event to detect when a row is "Unshared":

this.dataGridView.RowUnshared += (sender, args) => Debug.WriteLine("Unshared {0}", args.Row.Index);

The worst is that even a simple call to "grid.Rows[i]" is enough to unshare it... Look at the DatagridViewCollection's code:

 public DataGridViewRow this[int index] 
        {
            get 
            {
                DataGridViewRow dataGridViewRow = SharedRow(index);
...
                 // unshare row
                  DataGridViewRow newDataGridViewRow = (DataGridViewRow) dataGridViewRow.Clone();
...
                 this.DataGridView.OnRowUnshared(newDataGridViewRow);
fremag commented 6 years ago

I subscribed to RowUnshared event in LogExpert and every time I scroll (with mouse wheel), a row is unshared. I think the "Show bookmarks bubbles" doesn't help. When I unchecked the feature, the event was triggered less. Maybe this feature should be off by default, I think it would help a bit.

zarunbal commented 6 years ago

Hey @fremag really great work! Thanks you alot for your tests and verfications. I'll check your changes and try to optimize LogExpert accordingly.

As far as I see following points should be currently considered / changed:

fremag commented 6 years ago

@zarunabl: thanks a lot, I use LogExpert daily so I'm happy to help and thank you for your time spent on this project. Unfortunately I don't have a pull request that solves the memory issue.

Having a deeper look at DataGridView control, I feel like DataGridView can display millions of line but only if you do nothing: just look at them and click on the scroll bar.

I wonder if the solution is not to count the unshared rows with the event and after a limit is reached, reset it's data with a RowCount = 0 / Clear and then "reload" the file. Not perfect but could work.

Pr0metheus2 commented 6 years ago

I am against "reloading" the file, I sometimes connecting via slow VPN connection and loading of couple of MB log file is really pain.

fremag commented 6 years ago

Right, I meant rebuild the datagridview like if you just loaded the file. So the row would be in shared state and not consume too much memory.

zarunbal commented 6 years ago

@Pr0metheus2 If there is a new behavior to clear the unshared rows, this will until its really ready and fully tested a opt in option!

My current idea would be to clear the DataGrid and reload the latest displayed lines from the buffers. So the file won't get fully realoaded maybe the last few lines if the buffer doesn't have the right lines. (This idea has to be verified if it would work ;) )

aijanai commented 3 years ago

Hi, the bug is still here

jrouzies-fr commented 2 years ago

Any news to fix this bug?

This Log viewer is nearly the best, but this leak make it unusable on Production servers

Hirogen commented 2 years ago

Yeah, I should have time to look at it, yesterday ran into the bug again!

TerraNqobile commented 2 years ago

Any news to fix this bug?

My Production servers where unusable because it took all the ram

Hirogen commented 2 years ago

Any news to fix this bug?

My Production servers where unusable because it took all the ram

sorry not yet, currently recovering from surgery, this issue is currently top prio in analyzing, sadly it's not so easy to pin down

Hirogen commented 2 years ago

Anybody who encounters this problem, how many Highlighters did you have? I currently trying to find a solution and it seems that having a lot of highlighters, might be a problem, since the rows who are different become "unshared".

Hirogen commented 2 years ago

1.9.0 has been released (https://github.com/zarunbal/LogExpert/releases/tag/v1.9.0), please see if this helps with the memory leak

jrouzies-fr commented 2 years ago

1.9.0 has been released (https://github.com/zarunbal/LogExpert/releases/tag/v1.9.0), please see if this helps with the memory leak

So it seems to have fixed it, I have 6 files opened since release, with highlights (and even negative highlight to not have white background), and the RAM usage is still around 40 MB.

Hirogen commented 2 years ago

lets just a wait a few more days before we call it fixed ;D