I replied that when I looked at my current results, I'm not seeing those
kinds of percentages.
-- Blake Sullivan
Ed Burns said the following On 4/1/2010 3:19 PM PT:
> This is an omnibus reply to a big private email thread that I think
> would better be discussed on the Mojarra email list. I've Bcc'd the
> people that started the thread and who may not be on
> dev_at_javaserverfaces.dev.java.net
>
>
>>>>>> On Thu, 18 Mar 2010 22:36:33 +0100, Dünnenberger Hanspeter said:
>>>>>>
>
> HD> tried to contact you on the chat - but actually that might work
> HD> better by email.
>
> UIComponentBase.getFacesContext() says
>
> HD> // PENDING(edburns): we can't use the cache ivar because we
> HD> // don't always know when to clear it. For example, in the
> HD> // "save state in server" case, the UIComponent instances stick
> HD> // around between requests, yielding stale facesContext
> HD> // references. If there was some way to clear the facesContext
> HD> // cache ivar for each node in the tree *after* the
> HD> // render-response phase, then we could keep a cache ivar. As
> HD> // it is now, we must always use the Thread Local Storage
> HD> // solution.
>
> HD> return FacesContext.getCurrentInstance();
>
> HD> In the comment you asked a way to clear the cache variable - well,
> HD> will UIComponentBase.saveState() allways be called - also with new
> HD> partial state saving?
>
> AS> I think that we should explore this possibility further since this might
> AS> provide a solution that would work with Trinidad's view root caching.
> AS> Note that even when the Trinidad view root caching optimization is used,
> AS> we still need to save state so that the state is available in the
> AS> session for replication/fail over purposes.
>
> [...]
>
>
>>>>>> On Fri, 19 Mar 2010 10:10:18 +0100, "Marinschek Martin (KCBC 111)" said:
>>>>>>
>
> MM> I absolutely agree with Hanspeter that this should go. There is two
> MM> issues why I think Mojarra uses FacesContext.getCurrentInstance():
>
> MM> 1) in the beginnning, Mojarra didn't really execute
> MM> saveState/restoreState, it just reused the component tree for the
> MM> next request - but this has been fixed way back
>
> Yes, as Hanspeter mentioned in his first email.
>
> MM> 2) in the portlet environment, there are two requests, an action
> MM> request and a render request. The portlet bridge might (wrongly)
> MM> decide to reuse the component
>
> [...]
>
>
>>>>>> On Wed, 31 Mar 2010 15:25:00 +0200, "Marinschek Martin (KCBC 111)" <martin.marinschek_at_credit-suisse.com> said:
>>>>>>
>
> [...]
>
> MM> and we could actually save 2-3% of application performance if this
> MM> was cached.
>
> [...]
>
> MM> Our solution for this now would be that the faces-context is cached,
> MM> but that the tag-handler resets the faces-context information
> MM> whenever it creates or updates the UIComponent instance.
>
> This would most likely be new public API, as you know.
>
> MM> Now, this should work even for the dumb portlet bridges. However, I
> MM> have just learned that Trinidad actually caches the full component
> MM> tree, and just re-uses it on the next request.
>
> MM> @Andy, will the tag-handlers then be reapplied, so that our strategy
> MM> works, or is not even this happening? In this case, we might run
> MM> into problems with this approach. Do you have a solution for this?
>
> And what about for JSP?
>
> AS> Another safer option might be for UIComponentBase to cache the
> AS> FacesContext for shorter time periods. For example,
> AS> UIComponentBase.encodeBegin() could cache, UIComponentBase.encodeEnd()
> AS> could clear the cache and UIComponent.getFacesContext() could first
> AS> check for a cached instance, and if not found fall back on
> AS> FacesContext.getCurrentInstance(). That seems like a much safer
> AS> solution that should provide a decent improvement - assuming that most
> AS> calls to UIComponent.getFacesContext() are coming from the
> AS> UIComponentBase.AttributeMaps.get() during rendering.
>
> I like this conservative approach very much as well.
>
> MM> yes - so why don't do it at all three locations:
>
> MM> 1) saveState
> MM> 2) ComponentHandler.apply for creating a new component and
> MM> 3) ComponentHandler.apply for finding an existing component
>
> MM> that should be pretty safe!
>
> B> component tree before we cleared out the references. One way to
> B> make the presence of stale references more apparent would be to move
> B> the FacesContext instance into an invalid state when disconnecting
> B> it from the ThreadLocal.
>
> We already have this in Mojarra. Every public method calls
> assertNotRealeased() first thing. This method does not assert(), it
> throws an ISE if it's released. Now, it might be possible a
> FacesContext could be not released, but still be divorced from its
> ThreadLocal, but I don't see how.
>
> B> Trinidad should be cleaning out the transient subtrees during
> B> state saving (in case the page author adding any properties
> B> that he doesn't expect to see on the next request), so I think
> B> we should be OK there.
>
> MM> ok - so my suggestion looks good
>
> Martin, your 1) 2) 3) suggestion you mean?
>
> MM> I believe that when a component is state-saved and it has a binding
> MM> attribute, we should call the binding attribute and pass null
> MM> through to the setter. With this, we might at least be able to
> MM> prevent the greatest damage. This would make a lot more sense than
> MM> the current behaviour of leaving this to the user to get right.
>
> Yes, I think this is safe, assuming that the binding attribute gets
> restored correctly, which it should.
>
> MM> @Ed: do you want me to open issues for this? do you agree with this
> MM> outcome? We can discuss some more on the EG-list of course...
>
> I have done so
> <https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1599>.
>
> Ed
>
>