[jsr-314-open-mirror] [jsr-314-open] [153-UIDataStateSaving] spec clarifications requested by Andy
Andy Schwartz
andy.schwartz at oracle.com
Thu Nov 4 12:10:33 EDT 2010
(Re-sending, this time hopefully as plain/text, since the JBoss archive
scrubbed my previous text/html response.)
On 11/3/10 4:00 PM, Leonardo Uribe wrote:
> Hi
>
> 2010/11/3 Ed Burns <edward.burns at oracle.com
> <mailto:edward.burns at oracle.com>>
>
> >>>>> On Tue, 19 Oct 2010 12:35:05 -0400, Andy Schwartz
> <andy.schwartz at oracle.com <mailto:andy.schwartz at 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
More information about the jsr-314-open-mirror
mailing list