Bug 51822 - Selecting a node triggers too many gui (JMeterGUIComponent) updates / configuration
Selecting a node triggers too many gui (JMeterGUIComponent) updates / configu...
Status: NEEDINFO
Product: JMeter
Classification: Unclassified
Component: Main
2.5
All All
: P2 enhancement with 1 vote (vote)
: ---
Assigned To: JMeter issues mailing list
:
Depends on:
Blocks: 51830
  Show dependency tree
 
Reported: 2011-09-15 13:13 UTC by benoit.wiart
Modified: 2011-12-31 15:27 UTC (History)
1 user (show)



Attachments
Save 1 invocation of GuiPackage #getCurrentGui (1.37 KB, patch)
2011-09-15 13:16 UTC, benoit.wiart
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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