On 11/3/10 4:00 PM, Leonardo Uribe wrote:
Hi

2010/11/3 Ed Burns <edward.burns@oracle.com>
>>>>> On Tue, 19 Oct 2010 12:35:05 -0400, Andy Schwartz <andy.schwartz@oracle.com> said:


AS> What happens is partial state saving is disabled?  Do we still implement
AS> this behavior and just save/restore the full state, or do we bail on the
AS> preserveRowComponentState behavior?

I have clarified that the preserveRowComponentState is not impacted by
the application partial state saving behavior.

I don't understand how this can be.  The documentation for preserveRowComponentState/rowStatePreserved is heavily dependent on partial state saving - eg. see all of the references to markInitialState(), which is only called when partial state saving is enabled.

It is certainly possible to support rowStatePreserved-like behavior for the full state saving case.  However, the implementation would have significant differences from behavior that is currently described in the doc.

While I agree that it would make sense for rowStatePreserved to be honored regardless of whether partial/full state saving is on, simply stating that:

Note that this delta state is saved regardless of whether or not partial state saving has been enabled or disabled for this application.

Is not nearly sufficient to cover the full state saving case.

Part of the problem is that the spec for rowStatePreserved is overly detailed in what implementations are required to do.  The spec is very explicit about how to implement this feature for the partial state saving case, but this same approach does not work for the full state saving case.  We either need to describe both cases in detail, or revise the specification to provide a higher-level description of the expected behavior, rather than explicit implementation instructions.


AS> Do we need to perform two separate traversals?  Woudn't it be more
AS> efficient to consolidate these into a single traversal?

Perhaps, but that's how Leonardo first implemented it and I am not
inclined to try to optimize it here.


Yes, it is possible. On the first implementation, I wanted to keep things simple
and understandable, so I keep them on different methods, but now the algorithm
is well understood.

Sure, that makes sense.

However, this is another case where the specification is being overly-specific in dictating implementation details that are better left unspecified.  The specification must describe the requirements in a way that allows implementors to make their own decisions about how best to balance complexity/performance - ie. the specification must not get in the way of an implementation that wants to implement this functionality in a more efficient manner.  The current wording is not appropriate.  This needs to be fixed for 2.1.

On a somewhat related note, the spec currently requires that we collect non-transient state first, followed by transient state.  This poses a non-obvious dilemma for a use case that I care about.  The use case is a bit obscure, so I'll start with some background...

Trinidad provides an (optional) optimization for keeping the current component tree around across requests.  This is enabled by the org.apache.myfaces.trinidad.CACHE_VIEW_ROOT [1] context parameter.  One potential pitfall with this optimization: without taking special steps to free up transient state referenced by the component tree, this transient state will survive across requests, which is a significant difference from the normal state saving behavior.

One way to limit the impact of this is for components that hold onto transient state to free up this state at the end of the request.  The most obvious time to do this is when saveState() is called.  Now that we are introducing a formal transient state solution, I would very much like to see this transient state releasing behavior implemented centrally - ie. it would very much help the view root caching optimization if we could discard transient state when (non-transient) state saving is performed.

This should be trivial to implement - eg. in Mojarra, ComponentStateHelper.saveState() could call transientState.clear().  However, if UIData's rowStatePreserved behavior requires that saveState() is called before saveTransientState(), this would prohibit us from clearing transient state during state saving, since this would break transient state saving in the UIData case.  Swapping the order around (save transient state first followed by non-transient state), would at least allow the possibility of implementing this in a way that would work well with Trinidad's view root caching.

It might make sense to consider whether we should formalize some of this in 2.2 - eg. we may want to consider promoting the view root caching optimization up to the JSF spec/implementations and standardizing the requirements for what components need to do to be compatible with this.  In the meantime, I would ask that we avoid introducing any new spec language that would thwart this functionality.

Oh, on yet another tangent... I noticed while looking at Mojarra's ComponentStateHelper that we have the following implementations for save/restoreTransientState():

    public void restoreTransientState(FacesContext context, Object state)
    {
        transientState = (Map<Object, Object>) state;
    }
    
    public Object saveTransientState(FacesContext context)
    {
        return transientState;
    }

Doesn't this mean that we'll end up sharing the same transient state map across all rows?  I don't see how this can possibly work.  Don't we need to make a copy of the state on the save rather than providing a reference to our internal transient state map?



EB> If the rolled up state saved during the call to markInitialState() is
EB> not null or empty,

AS> Since this is the "initial rolled up state" - ie. the full state,
AS> wouldn't this be non-null?

I'm not sure of the answer to this one.  Leonardo?


In theory, if UIData.markInitialState is called correctly, the "initial rolled up
state" is not null, so the check could be skipped. In the first versions of the
algorithm, the check was used to fallback to the previous algorithm, but now
this is done using a check for the property ( isPreserveRowComponentState() ).

Hrm... so seems like this will never be null.


EB> If the per-row state saved in step a. above is null, traverse the
EB> children and restore the child state using the initial rolled up state.

AS> Does the "per-row state saved in step a. above" refer to the stave saved
AS> for the row that we are leaving?  Or for the state that we saved the
AS> last time that visited the current row?

AS> That is, if we are on row 1 and setRowIndex(2) is called, does the
AS> "per-row state saved in step a" mean the state produced for row 1 before
AS> we adjusted the index, or the state that we previously saved away the
AS> last time we visited row 2?

AS> This is important to specify clearly.

It's the state before we adjusted the row index.  I've clarified that.

EB> If the per-row state saved in step a. above is not null, traverse the
EB> children and restore the child state using the state saved during step
EB> a., using the initial rolled up state only as a backup in the case
EB> that per-row state is not available.

AS> Okay, so I guess that "per-row state saved in step a. above" refers to
AS> the previously saved state for the current row.

No, it's the state before we adjusted the row index.  Leonardo, it is
vitally important that you and Andy and I agree on this.


The "per-row state saved in step a. above" in other words means the "delta"
state from the row that will be changed. If we are on row 1, to change to row 2
we need to restore the state of row 2, and check if it is null or not, If it is null
we use only the initial rolled up state, but if is not, we use both the initial
rolled up state and the "per-row state saved".

This makes sense.  Any time we being operating on a new row, we need to:

1.  Restore it to the initial state using the rolled up (full) state that we saved during markInitialState().  And...
2.  Apply delta state, if there is any.

Ed, this means that this clarification:

It's the state before we adjusted the row index.  I've clarified that.

Is incorrect and needs to be re-clarified. :-)

BTW, this is a case where full vs. partial state saving impacts the implementation.  If we implement rowStatePreserved behavior for the full state saving case, we would never do #2 above - ie. there would never be any delta state.  Instead, we would always save off the full state for each row on exit and restore the previously saved full state on re-entry.

 
AS> However, since this is delta state, before we apply this isn't it
AS> necessary to first restore the components to their initial state by
AS> restoring the full initial saved state?  If we don't restore the
AS> components to their initial state before applying the delta state,
AS> won't we run the risk that state from the previous row might bleed
AS> over into the current row?  (In the case where the state from the
AS> previous from was null, this won't be an issue.)

AS> Also, at some point is it necessary to tell the StateHelper that it
AS> needs to clear out any previously saved deltas so that it can start
AS> tracking deltas for the currently active row?  Or does that happen
AS> implicitly at some point, eg. when we restore the row state?


I answered my own question by taking a peek at the code.  I see that in Mojarra's UIData implementation that we explicitly call markInitialState() after restoring the initial/rolled up state and before applying deltas:

                component.clearInitialState();
                if (childInitialState != null)
                {
                    component.restoreState(facesContext, childInitialState);
                    component.markInitialState();
                    component.restoreState(facesContext, childState);
                }

This seems reasonable.

EB> If the per-row state saved in step b. above is null, traverse the
EB> children and restore the transient state by passing null to each
EB> child's
EB> UIComponent.restoreTransientState(javax.faces.context.FacesContext,
EB> java.lang.Object) method
EB>
EB> If the per-row state saved in step b. above is not null, traverse the
EB> children and restore the transient state from the state saved in step
EB> b. above, calling
EB> UIComponent.restoreTransientState(javax.faces.context.FacesContext,
EB> java.lang.Object) on each child, and passing the appropriate state.

AS> Sounds like we are doing the same thing whether or not the transient
AS> state is null, so perhaps we can simplify the wording.

Perhaps, but I'm concerned with getting it wrong.  The current text was
approved by Leonardo so I'm inclined to go with it.


In few words, it just say how to restore the "transient state". It is possible in this
part I just forget to simplify things.

The time I wrote that part I was thinking if there was some kind of manipulation
on transient state properties before build view time, those values should be
"propagated" to child rows, but the intention of this state is to have a very
short life time (vdl build time or render time). If a value is necessary
to be stored more than the current request, use StateHelper. After thinking about
it carefully, the conclusion was that is not necessary, so we can just assume a
"null" or "empty" state for the transient map.

Hrm... so does that mean that we can simplify the above wording?  I think we should if we can - no need to make the doc more complicated than it already is.
 
AS> Few other questions:

AS> 1.  Leonardo raised an issue regarding the timing of when
AS> markInitialState() is called - ie. that markInitialState() needs to be
AS> called on the parent before the children - otherwise UIData won't be
AS> able to capture the full initial state of its children.  How did we
AS> solve this problem?  Are there spec changes relating to this?  Did we
AS> find a way to do this that doesn't require introducing yet another full
AS> tree traversal?

I don't know.



I see that we have added an extra traversal, in FaceletViewHandlingStrategy:

     private void markInitialState(final UIComponent component)
     {
         component.markInitialState();
         for (Iterator<UIComponent> it = component.getFacetsAndChildren() ; it.hasNext() ; ) {
             UIComponent child = it.next();
             if (!child.isTransient()) {
                 markInitialState(child);
             }
         }
     }    


This sort of work represents a hidden extra tax on the lifecycle, particularly for views with large component trees.  At this point I have lost track of how many component tree traversals JSF requires.  Unfortunately we are still in the process of doing our 1.2 vs. 2.0 performance testing here, so I am not yet in a position to say how significant the extra overhead that we have added in 2.0 (and now 2.1) is - ie. I don't really know whether I should object to yet another component tree traversal or not.  All I can say is that anytime we add additional processing that involves visiting every component, this makes me nervous.



It is possible to prevent the full tree traversal, but to do that it is necessary to
change all code that uses PostAddToViewEvent to relocate components, and that
means refactor composite component solution as is. This was done on MyFaces,
so there cc:insertChildren and cc:insertFacet does not use a Listener attached to
that event there, but Mojarra uses it.

Anyway, users could add components on PostAddToViewEvent to the tree, so
in theory the call to markInitialState() should be done after that event is handled.

For 2.2 I think we are going to need to take a closer look at how well partial state saving/markInitialState works for dynamically added subtrees.


AS> 2.  Regarding the name of the new property...
AS> is/setPreserveRowComponentState() is a bit of a mouthful.  Could we
AS> maybe shorten this to is/setRowStatePreserved()?

I like that a lot better.

AS> 3.  Are there cases where it might be useful to enable per-row transient
AS> state saving without also enabling non-transient state saving (which
AS> seems more expensive)?  I wonder whether it should be possible to enable
AS> these independently, in which case we may want to consider using an enum
AS> property instead of a boolean.  Also wondering whether transient row
AS> state saving should just be on by default or possibly always on.

Leonardo?


It is possible, but shouldn't that condition just depends on the component child itself ?.
For example, UIForm has submitted property that in the future will be stored in transient
map, so this component requires transient state be always on.

That's right.  A problem that we have is that it won't necessarily be clear to application developers when the are using a component that happens to make use of transient state and thus requires rowStatePreserved to be enabled.  It would be good if we could find some way to limit the chance that application developers will get this wrong.  Or, if not that, it would help if we could at least warn application developers in the event that they have missed this.

Andy

[1] http://myfaces.apache.org/trinidad/devguide/configuration.html#org.apache.myfaces.trinidad.CACHE_VIEW_ROOT