Fair enough, but as with most performance questions. actual numbers are of the essence. I've had thoroughly brilliant (so I thought) insights into how to make code faster that, after measuring, didn't do a darn thing. And since you're adding memory allocations (which, at higher server loads, will decrease performance), you'd want to see a convincing improvement.
As for "getValueBinding not being final", therefore overriding it is expected, I'd note that the EG here was not as thorough and far-seeing as you perhaps give us credit for being. :) It's not final 'cause no-one thought about the question one way or the other. Overriding getValueBinding() seems a bit odd to me. -- Adam On 12/9/05, Simon Kitching <[EMAIL PROTECTED]> wrote: > 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.txt > >> > >>> Avoid 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 > >> > >> > > > > > >
