voyent
Bad performance of MenuRenderer  XML
Forum Index -> Components
Author Message
Newlukai

Joined: 29/Nov/2006 00:00:00
Messages: 125
Offline


Hi there,

at the moment I'm profiling my application and I think I came across a serious performance problem of MenuRenderer especially when using Seam. Give me a try to explain ;)

The page I profiled contains some <h:selectOneMenu>s which contain about 1,500 selectItems (added together). As you might guess those selectItems aren't hard-coded in the xhtml markup. The values are provided by Seam components.

Code:
@Stateful
 @Scope(ScopeType.APPLICATION)
 @Name("selectMenuHelper")
 public class SelectOneMenuHelperAction implements SelectOneMenuHelper, Serializable {
   private static final long serialVersionUID = 8319080738882204480L;
 
   @In
   private transient ITestcaseService testcaseService;
   //some more services following
   
   private List<Testcase> testcases;
   private List<SelectItem> testcasesSI;
   //some more lists following
 
   
   public List<Testcase> getTestcases() {
     if(testcases == null) {
       testcases = testcaseService.getTestcases();
     }
     return testcases;
   }
   
   public List<SelectItem> getTestcasesSI() {
     if(testcasesSI == null) {
       getTestcases();
       
       testcasesSI = new LinkedList<SelectItem>();
       for(Testcase testcase : testcases) {
         testcasesSI.add(new SelectItem(testcase.getID(), testcase.getID() + " - " + testcase.getToken()));
       }
     }
     
     return testcasesSI;
   }
   
   //some more getters following
 }


There are two lists (testcases, testcasesSI) because I tested this

Code:
<h:selectOneMenu value="#{testactionDeveloper.testcase}" id="TCaseID">
   <f:selectItems value="#{selectMenuHelper.testcasesSI}" />
 </h:selectOneMenu>


vs. this

Code:
<ice:selectOneMenu value="#{testaction.TCaseID}" converter="javax.faces.Number" id="TCaseID">
   <s:selectItems value="#{selectMenuHelper.testcases}" var="testcase" label="#{testcase.ID} - #{testcase.token}" />
   <s:convertEntity />
 </ice:selectOneMenu>


to verify how much Seam's EntityConverter affects performance. At the moment I don't use this EntityConverter (i. e. the first xhtml markup is running), so following explanation is based on tests without it. E. g. this is a part from the tested xhtml page:

Code:
<h:panelGrid cellpadding="0" cellspacing="0" border="0" columns="10" rowClasses="tableRowHeader, tableRowContent" styleClass="testaction">
   <h:outputText value="#{ares_messages.label_testaction_ID}" />
   <h:outputText value="#{ares_messages.label_testaction_TDate}" />
   <h:outputText value="#{ares_messages.label_testaction_TCaseID}" />
   <h:outputText value="#{ares_messages.label_testaction_TesterUsrID}" />
   <h:outputText value="#{ares_messages.label_testaction_EnvID}" />
   <h:outputText value="#{ares_messages.label_testaction_SevID}" />
   <h:outputText value="#{ares_messages.label_testaction_EditorUsrID}" />
   <h:outputText value="#{ares_messages.label_testaction_DevUsrID}" />
   <h:outputText value="#{ares_messages.label_testaction_RevID}" />
   <h:outputText value="#{ares_messages.label_testaction_ChnglistID}" />
 
   <h:outputText value="#{testaction.ID}" />
   <h:outputText value="#{testaction.TDate}" />
 
   <h:selectOneMenu value="#{testactionDeveloper.testcase}" id="TCaseID">
     <f:selectItems value="#{selectMenuHelper.testcasesSI}" />
   </h:selectOneMenu>
 
   <h:outputText value="#{testaction.testerUsrID.ID}" />
   <h:outputText value="#{testaction.envID.hostname}" title="#{testaction.envID.OSVID} | #{testaction.envID.remark}" />
   <h:outputText value="#{testaction.sevID.descr}" />
   <h:outputText value="#{testaction.editorUsrID.ID}" />
 
   <h:selectOneMenu value="#{testactionDeveloper.developer}" id="devUsrID">
     <f:selectItems value="#{selectMenuHelper.developersSI}" />
   </h:selectOneMenu>
 
   <h:selectOneMenu value="#{testactionDeveloper.revision}" id="revID" partialSubmit="true">
     <f:selectItems value="#{selectMenuHelper.revisionsSI}" />
   </h:selectOneMenu>
 
   <s:decorate id="decorate_chnglistID" template="inc/decorateField.xhtml">
     <h:inputText value="#{testaction.chnglistID}" id="chnglistID" />
   </s:decorate>
 </h:panelGrid>


The snapshot I took from loading this page shows that the rendering of this page is the most time consuming task. I had a look at the method calls that occur during the render phase and saw that MenuRenderer.renderOptions() is very slow. Of course I downloaded the sources and inspected the code and I think there could be done some improvements.

MenuRenderer.renderOptions() calls renderOption() for every item. In renderOption() it's necessary to know, if the current item is marked as selected, so it's needed to call isValueSelected().
But there it happens: isValueSelected() calls getCurrentSelectedValues() (when there are no submitted values) and getCurrentSelectedValues() just returns the value of the given selectOneMenu, so it calls its getValue() method.

In an application which uses Seam the use of getValue() can be very time consuming:

Code:
public Object getValue() {
   if (this.value != null) {
     return (this.value);
   }
   
   ValueExpression ve = getValueExpression("value");
   if (ve != null) {
     try {
       return (ve.getValue(getFacesContext().getELContext()));
     } catch (ELException e) {
       throw new FacesException(e);
     }
   } else {
     return (null);
   }
 }


It's the first time the page is displayed, so this method evaluates the EL expression given in its "value" attribute. And as you can see in my xhtml markup it's a call to a Seam component. So Seam is invoked and Seam needs to lookup the corresponding Seam component, what is a slow task. And this is done about 1,500 times, once for every item/option.

I don't know if I made a serious mistake or if I misinterpreted something. But I think the performance of my application can be extremely increased by avoiding a lookup for a Seam component to render one selectItem in a selectOneMenu/selectManyMenu.

And if I'm right in claiming this, the next problem is to find out the one who should improve his code.
Is it an improvement which should be done by ICEfaces? A simple caching of the component's value until the rendering is finished would be enough.
Or should Sun modify UIOutput to cache its value once the ValueExpression is evaluated?
Perhaps both should do some improvement?

I don't know. What do you think about that?

Thanks in advance
Newlukai
Newlukai

Joined: 29/Nov/2006 00:00:00
Messages: 125
Offline


Since I had some time I modified MenuRenderer in a way that avoids those unnecessary lookups. After building and testing, the page is rendered after <1 second instead of about 4 seconds.

I attached the source code. I hope that someone takes a look and integrates this fix. I can't do it since I don't know enough about ICEfaces, i. e. whether my change affects other use cases or not.
 Filename MenuRenderer.java [Disk] Download
 Description
 Filesize 28 Kbytes
 Downloaded:  376 time(s)

mark.collette


Joined: 07/Feb/2005 00:00:00
Messages: 1692
Offline


To accept code patches, we need a signed contributor's agreement from you.

Also, I downloaded and examined the file you've attached, and it's identical to my version. Perhaps you've attached the wrong version of MenuRenderer.java?
[Email]
Newlukai

Joined: 29/Nov/2006 00:00:00
Messages: 125
Offline


mark.collette wrote:
Also, I downloaded and examined the file you've attached, and it's identical to my version. Perhaps you've attached the wrong version of MenuRenderer.java? 


Whoops. I've too many versions of ICEfaces here ;)
 Filename MenuRenderer.java [Disk] Download
 Description
 Filesize 30 Kbytes
 Downloaded:  323 time(s)

Newlukai

Joined: 29/Nov/2006 00:00:00
Messages: 125
Offline


mark.collette wrote:
To accept code patches, we need a signed contributor's agreement from you. 


? I just wanted the ICEfaces team to note that there is a performance problem. I don't know anything about the affects my change takes in other use cases, so I think a developer from ICEfaces should take care about this.
Newlukai

Joined: 29/Nov/2006 00:00:00
Messages: 125
Offline


I'm sorry. I've found out that the real bottleneck in my application was Seam's EntityConverter. This MenuRenderer patch doesn't lead to a noticeable performance improvement.
mark.collette


Joined: 07/Feb/2005 00:00:00
Messages: 1692
Offline


Well, I'll still try to look into making use of it. Thanks for taking the time :)
[Email]
nerlijma

Joined: 22/Oct/2007 00:00:00
Messages: 36
Offline


I still have perfomance issues on Internet Explorer 6 with big ice:menuBar (7 first level and 10 each sub level). FF does not have this issue. Is anybody facing this problem too?
Thanks
nerlijma

Joined: 22/Oct/2007 00:00:00
Messages: 36
Offline


I have created and issue for this, as I really think that this issue will stop any application that use a quite big menubar and IE6, from going to production

http://jira.icefaces.org/browse/ICE-3217
Newlukai

Joined: 29/Nov/2006 00:00:00
Messages: 125
Offline


Today I profiled my application again with ICEfaces 1.7.1. I don't know why, but it seems that anybody was interested in my proposal.

On several pages in my application there are multiple selectOneMenus rendered and they have many entries. While the menu is rendered MenuRenderer checks every entry if it belongs to the selected ones. The implementation of this check is time consuming since it asks the UIComponent for the selected values again and again, for every single entry. This request for the selected values uses JNDI. In my case one bean is queried over 400 times! After some code changes I got just about 70 requests for this bean. This simple change saved one second in rendering the page.

And once again I offer you my changes. Do I still have to sign this agreement? What are the consequences when I sign it?
 Filename RadioRenderer.java [Disk] Download
 Description
 Filesize 9 Kbytes
 Downloaded:  412 time(s)

 Filename MenuRenderer.java [Disk] Download
 Description
 Filesize 29 Kbytes
 Downloaded:  269 time(s)

Newlukai

Joined: 29/Nov/2006 00:00:00
Messages: 125
Offline


And the last file.
 Filename RadioRenderer.java [Disk] Download
 Description
 Filesize 9 Kbytes
 Downloaded:  269 time(s)

jguglielmin

Joined: 20/Jul/2007 00:00:00
Messages: 181
Offline


Since you are using Seam, did you use an @Factory to create the list (from your database) so that it is only done the one time (if it is null)?

Unless you want a fresh hit each time, you might want to review the scope of the bean that is creating these lists and also, if this list is static (each time you load the application) then consdier using a factory in components.xml to create the list for your selectItems. (especially since you are placing it in application scope??).

Newlukai

Joined: 29/Nov/2006 00:00:00
Messages: 125
Offline


I'll check this. It's a great hint. Thanks for that.

But with this "fix" addresses the fact that in order to render the menu those classes check for every single item if it's the selected one. But how do these classes get this information? They ask the UIComponent itself for every single item which the selected values are.
Firstly it would be more elegant to ask for the selected values only once for a menu, not n times (where n stands for the amount of items in this menu). Secondly you'll mostly find EL expressions in the value attribute of menus. And these expressions have to be resolved. That means that for every menu item the same EL expression is evaluated. Mostly this expression addresses a property of a bean and this bean has to be found. NamingContext is asked and a JNDI call is done.

I know that this behaviour isn't the only fact that reduces the performance of an application. I know that I have to check my code. But since I have large lists, this "fix" saved almost one second out of three. Now I'll check how to reduce the other two seconds ;)
And if the aspect of performance doesn't convince, perhaps you agree that this approach of asking the UIComponent every time for the selected values isn't elegant. OK, my fix isn't very elegant, too. But it just was a test to see what happens.

EDIT: All the beans providing those lists are session scope. The bigger lists contain dynamic data. There are only two or three lists that could be made "static" in session scope, but those lists are the smaller ones.

EDIT2: I found another improvement, this time it's a smaller one. menuRenderer contains a method renderSelect. In this method the size attribute is to be rendered. To render this attirbute all items in this menu are iterated and evaluated in order to count them. It's not necessary to evaluate all items in order to count them. But this one goes a step further: the size attribute always is rendered with value "1". It doesn't matter how many items are in this menu, the size always is "1".
The change of this doesn't improve the performance very much since all the items are already evaluated, but it's an unnecessary iteration.

I want to let you know that all this shouldn't be seen as a complaint from an annoyed customer ;). ICEfaces is great and I want to help to improve it.
 Filename MenuRenderer.java [Disk] Download
 Description
 Filesize 29 Kbytes
 Downloaded:  280 time(s)

jguglielmin

Joined: 20/Jul/2007 00:00:00
Messages: 181
Offline


We're always happy to have new contributors. Have you considered signing an agreement?

Meanwhile, if you haven't opened a jira for this already, why don't you do so, attach the suggested fixes (which requires the contributors agreement), reference this forum posting and categorize it as an improvement.
mark.collette


Joined: 07/Feb/2005 00:00:00
Messages: 1692
Offline


Sorry we had not made this change before. As it stands, we're currently working on ICEfaces 1.7.2, which is specifically all about improving performance. So, I've made a Jira for this:

http://jira.icefaces.org/browse/ICE-3482

[Email]
 
Forum Index -> Components
Go to:   
Powered by JForum 2.1.7ice © JForum Team