Bug 59258 - GUI Mode : OOM Protection for View Results Tree or View Results in Table
Summary: GUI Mode : OOM Protection for View Results Tree or View Results in Table
Status: RESOLVED DUPLICATE of bug 60687
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 2.13
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-31 21:08 UTC by Philippe Mouawad
Modified: 2017-03-08 21:01 UTC (History)
1 user (show)



Attachments
Patch for the issue (5.66 KB, patch)
2016-03-31 21:19 UTC, Philippe Mouawad
Details | Diff
Patch that does not add SampleResults anymore and displays a warning (12.25 KB, patch)
2016-04-01 19:20 UTC, Philippe Mouawad
Details | Diff
Screenshot showing the warning on ViewResultsTree (474.33 KB, image/png)
2016-04-01 19:21 UTC, Philippe Mouawad
Details
Screenshot showing the warning on ViewResultsTable (581.20 KB, image/png)
2016-04-01 19:22 UTC, Philippe Mouawad
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Mouawad 2016-03-31 21:08:58 UTC
As you know, unfortunately and despite all the warnings and best practices we wrote, a lot of users still use GUI Mode when Load Testing.
This is an issue when Listener such as View Results Tree or View Results in Table are used.

This ends usually with "abnormal and wrong" slow response times before an OOM occurs.

This issue is one of the major factor for JMeter being felt as "unstable" I think.

I propose the following solution that I have already developed

Add 2 user properties:
#view.results.table.max_rows=5000
#view.results.tree.max_nodes=5000

In the "View Results Tree " , when number of nodes in the tree reaches the max, we remove the first node in the Tree.
In the "View Results in Table" , when number of rows in the table reaches the max, we remove the first row in the table.

So we limit this way the number of nodes and rows in those listeners.
Comment 1 Philippe Mouawad 2016-03-31 21:19:44 UTC
Created attachment 33716 [details]
Patch for the issue
Comment 2 Vladimir Sitnikov 2016-03-31 22:27:50 UTC
Philippe, are you aware that both removals are "remove the first element out of arraylist"? Those seem to be pathological cases.

ObjectTableModel stores model in ArrayList, while DefaultTreeModel stores model in Vector.

Have you tested which overhead creates that node removal?
I'm not fond of running long operations under synchronization.
Comment 3 Philippe Mouawad 2016-04-01 07:03:25 UTC
(In reply to Vladimir Sitnikov from comment #2)
> Philippe, are you aware that both removals are "remove the first element out
> of arraylist"? Those seem to be pathological cases.

Can you clarify what you mean ?


> 
> ObjectTableModel stores model in ArrayList, while DefaultTreeModel stores
> model in Vector.
> 
> Have you tested which overhead creates that node removal?
No.
As for me anyway Load Testing in GUI mode is evil.
I am just here protecting JMeter from OOM

> I'm not fond of running long operations under synchronization.
What other solution do we have ?
Do this only protection part every N Inserts ?
Comment 4 Sebb 2016-04-01 08:19:19 UTC
I think Vladimir means that removal of the first element of those data structures is an expensive operation.

Therefore the 'cure' might cause other problems.

As to the solution, my preference is to not try to fix it through code.

However of course it would be quite cheap to start dropping entries when a limit is reached.
Comment 5 Philippe Mouawad 2016-04-01 08:25:17 UTC
(In reply to Sebb from comment #4)
> I think Vladimir means that removal of the first element of those data
> structures is an expensive operation.

Ok I agree

> 
> Therefore the 'cure' might cause other problems.
> 
> As to the solution, my preference is to not try to fix it through code.

What do you propose then ?
> 
> However of course it would be quite cheap to start dropping entries when a
> limit is reached.

Isn't it what my patch does ?
If it's cheap , then why not do it this way ?
Comment 6 Sebb 2016-04-01 08:57:17 UTC
(In reply to Philippe Mouawad from comment #5)
> (In reply to Sebb from comment #4)
> > I think Vladimir means that removal of the first element of those data
> > structures is an expensive operation.
> 
> Ok I agree
> 
> > 
> > Therefore the 'cure' might cause other problems.
> > 
> > As to the solution, my preference is to not try to fix it through code.
> 
> What do you propose then ?

Continue with education.

> > 
> > However of course it would be quite cheap to start dropping entries when a
> > limit is reached.
> 
> Isn't it what my patch does ?
> If it's cheap , then why not do it this way ?

Sorry, meant to write 'samples' not 'entries' - i.e. don't add another entry if the list is full. That should be cheap and solves the OOM.
Comment 7 Philippe Mouawad 2016-04-01 09:01:37 UTC
(In reply to Sebb from comment #6)
> (In reply to Philippe Mouawad from comment #5)
> > (In reply to Sebb from comment #4)
> > > I think Vladimir means that removal of the first element of those data
> > > structures is an expensive operation.
> > 
> > Ok I agree
> > 
> > > 
> > > Therefore the 'cure' might cause other problems.
> > > 
> > > As to the solution, my preference is to not try to fix it through code.
> > 
> > What do you propose then ?
> 
> Continue with education.

Unfortunately, I am not sure it's enough.
> 
> > > 
> > > However of course it would be quite cheap to start dropping entries when a
> > > limit is reached.
> > 
> > Isn't it what my patch does ?
> > If it's cheap , then why not do it this way ?
> 
> Sorry, meant to write 'samples' not 'entries' - i.e. don't add another entry
> if the list is full. That should be cheap and solves the OOM.

Yes that's another way to fix it.
But if you take a long running test:
- I suppose user would be more interested in last samples than old ones
- He might think JMeter is stuck, which ends up the same from User perception : "JMeter is unstable, my test is stuck ...."
Comment 8 Philippe Mouawad 2016-04-01 09:04:50 UTC
Hi sebb,
Just to illustrate, first question now on stackoverflow:
http://stackoverflow.com/questions/tagged/jmeter

Jmeter freezes with a CPU of 100%


Search in it and on our mailing list, I bet you give tens of entries
Comment 9 Sebb 2016-04-01 09:57:04 UTC
(In reply to Philippe Mouawad from comment #8)
> Hi sebb,
> Just to illustrate, first question now on stackoverflow:
> http://stackoverflow.com/questions/tagged/jmeter
> 
> Jmeter freezes with a CPU of 100%
> 
> 
> Search in it and on our mailing list, I bet you give tens of entries

Yes, there probably are.
There are lots of entries about other common problems which would be solved if people read the manual.

I agree that if we can make a simple change to JMeter that makes it easier to use and does not compromise performance then we should consider doing it.

However in this case any changes are likely to be intrusive, so the trade-off is not worth it.

Also, the test will still reach saturation earlier than it would do if they had not used the Listener, so they are then going to complain that JMeter does not scale. I think it's just postponing the problem.
Comment 10 Philippe Mouawad 2016-04-01 11:25:07 UTC
(In reply to Sebb from comment #9)
> (In reply to Philippe Mouawad from comment #8)
> > Hi sebb,
> > Just to illustrate, first question now on stackoverflow:
> > http://stackoverflow.com/questions/tagged/jmeter
> > 
> > Jmeter freezes with a CPU of 100%
> > 
> > 
> > Search in it and on our mailing list, I bet you give tens of entries
> 
> Yes, there probably are.
> There are lots of entries about other common problems which would be solved
> if people read the manual.

Yes, but if we can make it OOTB then why not
> 
> I agree that if we can make a simple change to JMeter that makes it easier
> to use and does not compromise performance then we should consider doing it.
> 
> However in this case any changes are likely to be intrusive, so the
> trade-off is not worth it.

I don't share this opinion. 
> 
> Also, the test will still reach saturation earlier than it would do if they
> had not used the Listener, so they are then going to complain that JMeter
> does not scale. 

But that won't be true. While currently saying that JMeter does not scale with GUI Mode + View Results Tree is not strictly wrong.


>I think it's just postponing the problem.


As a conclusion , maybe Vladimir has a brilliant idea that could help here.
If not then I suggest 2 options:
- We use my patch and accept drawback
- We use your option (drop additional sampler) AND add a warning message on GUI to say that this is in place
Comment 11 Sebb 2016-04-01 11:48:24 UTC
(In reply to Philippe Mouawad from comment #10)
> (In reply to Sebb from comment #9)
> > (In reply to Philippe Mouawad from comment #8)
> > > Hi sebb,
> > > Just to illustrate, first question now on stackoverflow:
> > > http://stackoverflow.com/questions/tagged/jmeter
> > > 
> > > Jmeter freezes with a CPU of 100%
> > > 
> > > 
> > > Search in it and on our mailing list, I bet you give tens of entries
> > 
> > Yes, there probably are.
> > There are lots of entries about other common problems which would be solved
> > if people read the manual.
> 
> Yes, but if we can make it OOTB then why not

Because of the disadvantages.

> > 
> > I agree that if we can make a simple change to JMeter that makes it easier
> > to use and does not compromise performance then we should consider doing it.
> > 
> > However in this case any changes are likely to be intrusive, so the
> > trade-off is not worth it.
> 
> I don't share this opinion.

Are you saying that the changes are not intrusive, or that even though they are intrusive, it's worth the disadvantages?

> > 
> > Also, the test will still reach saturation earlier than it would do if they
> > had not used the Listener, so they are then going to complain that JMeter
> > does not scale. 
> 
> But that won't be true. 

On the contrary, it is true that using one of these Listeners will reduce the max throughput that JMeter can achieve.
Maybe not in all tests, but it will be important in some.

Using extra storage can affect the max possible throughput.
Using extra processing to implement the limiting also affects the max throughput.

> While currently saying that JMeter does not scale
> with GUI Mode + View Results Tree is not strictly wrong.

Not sure what that means.

> 
> >I think it's just postponing the problem.
> 
> 
> As a conclusion , maybe Vladimir has a brilliant idea that could help here.

Maybe.

> If not then I suggest 2 options:
> - We use my patch and accept drawback

-1

> - We use your option (drop additional sampler) AND add a warning message on
> GUI to say that this is in place

Even though I suggested it, I don't think it's a viable idea.
Comment 12 Philippe Mouawad 2016-04-01 19:20:29 UTC
Created attachment 33718 [details]
Patch that does not add SampleResults anymore and displays a warning
Comment 13 Philippe Mouawad 2016-04-01 19:21:33 UTC
Created attachment 33719 [details]
Screenshot showing the warning on ViewResultsTree
Comment 14 Philippe Mouawad 2016-04-01 19:22:05 UTC
Created attachment 33720 [details]
Screenshot showing the warning on ViewResultsTable
Comment 15 Philippe Mouawad 2016-04-01 19:24:59 UTC
(In reply to Sebb from comment #11)
> (In reply to Philippe Mouawad from comment #10)
> > (In reply to Sebb from comment #9)
> > > (In reply to Philippe Mouawad from comment #8)
> > > > Hi sebb,
> > > > Just to illustrate, first question now on stackoverflow:
> > > > http://stackoverflow.com/questions/tagged/jmeter
> > > > 
> > > > Jmeter freezes with a CPU of 100%
> > > > 
> > > > 
> > > > Search in it and on our mailing list, I bet you give tens of entries
> > > 
> > > Yes, there probably are.
> > > There are lots of entries about other common problems which would be solved
> > > if people read the manual.
> > 
> > Yes, but if we can make it OOTB then why not
> 
> Because of the disadvantages.
> 
> > > 
> > > I agree that if we can make a simple change to JMeter that makes it easier
> > > to use and does not compromise performance then we should consider doing it.
> > > 
> > > However in this case any changes are likely to be intrusive, so the
> > > trade-off is not worth it.
> > 
> > I don't share this opinion.
> 
> Are you saying that the changes are not intrusive, or that even though they
> are intrusive, it's worth the disadvantages?

Even if they are intrusive it's worth the disadvantage.
Without protection : Results are KO and JMeter crashes
With protection : Results are KO, user is informed and JMeter does not crash

> 
> > > 
> > > Also, the test will still reach saturation earlier than it would do if they
> > > had not used the Listener, so they are then going to complain that JMeter
> > > does not scale. 
> > 
> > But that won't be true. 
> 
> On the contrary, it is true that using one of these Listeners will reduce
> the max throughput that JMeter can achieve.
> Maybe not in all tests, but it will be important in some.
> 
> Using extra storage can affect the max possible throughput.
> Using extra processing to implement the limiting also affects the max
> throughput.
> 
> > While currently saying that JMeter does not scale
> > with GUI Mode + View Results Tree is not strictly wrong.
> 
> Not sure what that means.

Currently, if someone uses GUI mode with View Results Tree for load testing, JMeter does not scale. Although this is against best practices we documented, this is possible.
With protection it is not anymore.

> 
> > 
> > >I think it's just postponing the problem.
> > 
> > 
> > As a conclusion , maybe Vladimir has a brilliant idea that could help here.
> 
> Maybe.
> 
> > If not then I suggest 2 options:
> > - We use my patch and accept drawback
> 
> -1
> 
> > - We use your option (drop additional sampler) AND add a warning message on
> > GUI to say that this is in place
> 
> Even though I suggested it, I don't think it's a viable idea.

I implemented the 2 options , I attached the patch. 
Frankly I think it is an improvement. 
I am really tired of seeing people fall into this trap too easily.
Comment 16 Philippe Mouawad 2016-04-03 12:40:59 UTC
This PR (https://github.com/apache/jmeter/pull/179) is a first try to base protection on memory estimation.
Memory estimation of SampleResult is too positive I think, it does only take into account the AssertionResult and SampleResult response data, not the properties size. This could be improved.
Comment 17 Philippe Mouawad 2017-03-08 21:01:31 UTC

*** This bug has been marked as a duplicate of bug 60687 ***