Hi Adam, Thanks very much for taking a look at this patch.
I agree that when explicitly avoiding a return value the patch causes two String "header" objects and one char array to be created: a string header + char-array when the prefix is added, and then a string header (but no data buffer) when the prefix is stripped off again.
There's no penalty, though, when put is called without the special prefix, ie the extra objects are only created during UIComponent initialisation.
Without the patch, any attempt to set an attribute via the put method will first look for a getter method. In 95% of cases, a getter method will be found (most UIComponents have setter methods for allowed JSP attributes). In this case, a call to that getter is made via Method.invoke. All the getter methods call getValueBinding, which does a HashMap lookup.
Invocations via Method.invoke are much faster than they used to be, but still worth avoiding if possible. Likewise HashMap lookups.
I'm not really pushing this patch as a major performance gain though :-). It's a nice little improvement I think (though I should try to back that up with real measurements!). Far more important to me is avoiding unexpected calls to getValueBinding. This method is not final, so presumably there is the intention for custom components to be able to override it - however practically that's difficult when the method can be invoked while the component is only half initialised, and the method cannot know whether that is the case or not.
Cheers, Simon Adam Winer wrote:
This seems of questionable utility for performance. On the one hand, yes, you're avoiding invoking the ValueBinding. On the other hand, you're now creating somewhere around three bonus objects (two String objects and one char array) every time you set *any* attribute, value-bound or not. (Object allocation is not as cost-free as some would have us believe, especially under heavy server loads). -- Adam On 12/8/05, Simon Kitching (JIRA) <[email protected]> wrote:[ http://issues.apache.org/jira/browse/MYFACES-920?page=all ] Simon Kitching updated MYFACES-920: ----------------------------------- Attachment: _ComponentAttributesMap.java.patch.txt UIComponentTagUtils.java.patch.txtAvoid invoking value-bindings during component initialisation ------------------------------------------------------------- Key: MYFACES-920 URL: http://issues.apache.org/jira/browse/MYFACES-920 Project: MyFaces Type: Improvement Components: General Reporter: Simon Kitching Attachments: UIComponentTagUtils.java.patch.txt, _ComponentAttributesMap.java.patch.txt When a component is created by a JSP tag, calls to setStringProperty, setBooleanProperty, etc are used to copy the JSP tag attributes onto the new component. (NB: other view implementations will be doing something similar I'm sure). The setStringProperty et. al. eventually call component.getAttributes().put(propName, value) Unfortunately, the Map.put method is defined to return the old value of the property. While the specs are a little vague on whether a return value is actually required as far as I can see, both MyFaces and Sun do try to return the old value. But this means calling the getter method on the component, which typically calls getValueBinding(). During component initialisation, this call to the component's getter method and thence to getValueBinding isn't actually *too* bad; the binding will never be found because each attribute is initially null, and is set only once. However it's still inefficient; it's a call via reflection, a HashMap lookup, etc. that is all completely unnecessary as the return value will (a) always be null, and (b) is ignored. This behaviour *is* a major pain for anyone (like me) who would like to override the getValueBinding method for a component; it gets called once for each attribute of the JSP tag, and during a time when the component is not completely initialised. And there's no way to tell when component initialisation has completed as far as I know. Attached is a proposed solution. The UIComponent.getAttributes method returns a custom (private) Map implementation already. It isn't possible to add any methods to this, as it's in the API namespace. Instead I've modified the put method so that if the provided propName has a special prefix then the method doesn't bother to fetch and return the old value. Updating the setStringProperty/setBooleanProperty/etc methods to add the prefix then solves the issue. What do people think of this? Is this ok to commit? NB: I will be creating some unit tests to go along with this, to prove that the patch does what it says. I don't have time just now, though, and wanted to post this before I left for the weekend. NB2: If this does go in (or some other solution to this problem) I've got a rather cool use for it. A "backingBean" attribute can then be defined on a component, and via a customised ValueBinding, any property on the component that supports value-binding will automatically try to fetch the data from an identically-named property on the specified backingBean for the component. It's very useful when dealing with components with lots of bindable properties, like a UIData - or my custom extension of that class, which has even more properties.-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
