Bug 51822

Summary:	Selecting a node triggers too many gui (JMeterGUIComponent) updates / configuration
Product:	JMeter - Now in Github	Reporter:	benoit.wiart
Component:	Main	Assignee:	JMeter issues mailing list <issues>
Status:	NEEDINFO ---
Severity:	enhancement	CC:	p.mouawad
Priority:	P2
Version:	2.5
Target Milestone:	---
Hardware:	All
OS:	All
Bug Depends on:
Bug Blocks:	51830
Attachments:	Save 1 invocation of GuiPackage #getCurrentGui

Description benoit.wiart 2011-09-15 13:13:07 UTC

For a test plan with 2 samplers
	Sampler A
	Sampler B

Switching from A to B will trigger too many gui (JMeterGUIComponent) updates / configuration
Precisely :
	1 JMeterGUIComponent#modifyTestElement for A
	3 JMeterGUIComponent#modifyTestElement for B
	4 JMeterGUIComponent#configure for B
	3 JMeterGUIComponent#clearGui for B

The root cause is GuiPackage #getCurrentGui being call 3 times
	EditCommand : 2 times
	MenuFactory#addFileMenu : 1

For components which do costly computation in JMeterGUIComponent#configure (eg : WebServiceSamplerGui) this bug introduce a lag the jmeter gui usage.
If the component displays a popup in JMeterGUIComponent#configure, it will be displayed 3 times…

I think we should review the way GuiPackage #getCurrentGui is used and at least rename it because this method is doing much more than getting the current gui.

The first patch does not correct the root cause but save 1 invocation.

Comment 1 benoit.wiart 2011-09-15 13:16:00 UTC

Created attachment 27501 [details]
Save 1 invocation of GuiPackage #getCurrentGui

Save 1 invocation of GuiPackage#getCurrentGui

Benoit Wiart
Ubik Ingénierie
www.ubik-ingenierie.com

Comment 2 benoit.wiart 2011-09-16 19:45:49 UTC

GuiPackage#updateCurrentGui is almost identical to GuiPackage#getCurrentGui except it does not call clearGui

What could be done :
  change updateCurrentGui to match getCurrentGui (add clearGui and try-catch)
  change getCurrentGui to simply return the current gui (nothing more)

Some call sites of getCurrentGui rely on the fact it will "save" the current gui
eg GuiPackage#localeChanged or JMeterTreeModel#addComponent
they must be updated to call updateCurrentGui

I have a working prototype which do not regress on bugs (43122
30120, 41078, 39427, 50221, 42346) or revisions (325250, 324131) 

Dear Jmeter maintainers let me know what you think about that...

Benoit Wiart

Comment 3 Sebb 2011-09-16 21:03:35 UTC

(In reply to comment #2)
> GuiPackage#updateCurrentGui is almost identical to GuiPackage#getCurrentGui
> except it does not call clearGui
> 
> What could be done :
>   change updateCurrentGui to match getCurrentGui (add clearGui and try-catch)
>   change getCurrentGui to simply return the current gui (nothing more)
> 
> Some call sites of getCurrentGui rely on the fact it will "save" the current
> gui
> eg GuiPackage#localeChanged or JMeterTreeModel#addComponent
> they must be updated to call updateCurrentGui

Please raise a separate issue for this.

Comment 4 benoit.wiart 2011-09-16 21:52:52 UTC

(In reply to comment #3)
> (In reply to comment #2)
> > GuiPackage#updateCurrentGui is almost identical to GuiPackage#getCurrentGui
> > except it does not call clearGui
> > 
> > What could be done :
> >   change updateCurrentGui to match getCurrentGui (add clearGui and try-catch)
> >   change getCurrentGui to simply return the current gui (nothing more)
> > 
> > Some call sites of getCurrentGui rely on the fact it will "save" the current
> > gui
> > eg GuiPackage#localeChanged or JMeterTreeModel#addComponent
> > they must be updated to call updateCurrentGui
> 
> Please raise a separate issue for this.

Hi Sebb

My comment 2 seems out of context...
BUT the change on updateCurrentGui and getCurrentGui is a way to correct this issue by removing the 'extra work' done in getCurrentGui.

Unless you really want to, I don't think it should be part of another bug

Comment 5 Sebb 2011-09-17 00:05:23 UTC

Seems to me that a better solution to the WebServiceSamplerGui.configure issue would be to move the expensive computation elsewhere.

The configure() method was intended for copying the TestElement fields to the GUI, not for doing GUI display manipulation.

Comment 6 benoit.wiart 2011-09-17 06:42:17 UTC

(In reply to comment #5)
> Seems to me that a better solution to the WebServiceSamplerGui.configure issue
> would be to move the expensive computation elsewhere.
> 
> The configure() method was intended for copying the TestElement fields to the
> GUI, not for doing GUI display manipulation.

I agree, we could also add some caching in wdslhelper

But we should also correct the multiples calls to JMeterGUIComponent#modifyTestElement, JMeterGUIComponent#configure, JMeterGUIComponent#clearGui done through getCurrentGui

Tell me if you don't want to impact getCurrentGui / updateCurrentGui : then you can only apply the first patch.
Otherwise I will create a patch from my prototype.

I'm waiting for your 'GO / NOGO'

Comment 7 Sebb 2011-09-17 10:20:57 UTC

(In reply to comment #1)
> Created attachment 27501 [details]
> Save 1 invocation of GuiPackage #getCurrentGui
> 
> Save 1 invocation of GuiPackage#getCurrentGui
> 

Thanks, that should help.

URL: http://svn.apache.org/viewvc?rev=1171938&view=rev
Log:
Bug 51822 - (part 1) save 1 invocation of GuiPackage#getCurrentGui

Modified:
   jakarta/jmeter/trunk/src/core/org/apache/jmeter/gui/action/EditCommand.java
   jakarta/jmeter/trunk/xdocs/changes.xml

Comment 8 Philippe Mouawad 2011-12-02 21:43:08 UTC

Hello Benoit,
Can you provide the patch you describe in comment 1 and 6  ?

I change this to an enhancement.

Regards
Philippe

Comment 9 The ASF infrastructure team 2022-09-24 20:37:46 UTC

This issue has been migrated to GitHub: https://github.com/apache/jmeter/issues/2536