<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
On 11/3/10 4:00 PM, Leonardo Uribe wrote:
<blockquote
cite="mid:AANLkTi=duVmXfOM3rn0Wn+vn_TZKTzLrtu2h=quZDkny@mail.gmail.com"
type="cite">Hi<br>
<br>
<div class="gmail_quote">2010/11/3 Ed Burns <span dir="ltr"><<a
moz-do-not-send="true" href="mailto:edward.burns@oracle.com">edward.burns@oracle.com</a>></span><br>
<blockquote class="gmail_quote"
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">>>>>>
On Tue, 19 Oct 2010 12:35:05 -0400, Andy Schwartz <<a
moz-do-not-send="true" href="mailto:andy.schwartz@oracle.com">andy.schwartz@oracle.com</a>>
said:<br>
<br>
<br>
AS> What happens is partial state saving is disabled? Do we still
implement<br>
AS> this behavior and just save/restore the full state, or do we
bail on the<br>
AS> preserveRowComponentState behavior?<br>
<br>
I have clarified that the preserveRowComponentState is not impacted by<br>
the application partial state saving behavior.<br>
</blockquote>
</div>
</blockquote>
<br>
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.<br>
<br>
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.<br>
<br>
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:<br>
<br>
<blockquote type="cite">Note that this delta state is saved regardless
of whether or not partial state saving has been enabled or disabled for
this application.</blockquote>
<br>
Is not nearly sufficient to cover the full state saving case.<br>
<br>
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.<br>
<br>
<br>
<blockquote
cite="mid:AANLkTi=duVmXfOM3rn0Wn+vn_TZKTzLrtu2h=quZDkny@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">AS>
Do we need to perform two separate traversals? Woudn't it be more<br>
AS> efficient to consolidate these into a single traversal?<br>
<br>
Perhaps, but that's how Leonardo first implemented it and I am not<br>
inclined to try to optimize it here.<br>
<br>
</blockquote>
<div><br>
Yes, it is possible. On the first implementation, I wanted to keep
things simple<br>
and understandable, so I keep them on different methods, but now the
algorithm <br>
is well understood.<br>
</div>
</div>
</blockquote>
<br>
Sure, that makes sense.<br>
<br>
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.<br>
<br>
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...<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
Oh, on yet another tangent... I noticed while looking at Mojarra's
ComponentStateHelper that we have the following implementations for
save/restoreTransientState():<br>
<br>
<blockquote type="cite"> public void
restoreTransientState(FacesContext context, Object state)<br>
{<br>
transientState = (Map<Object, Object>) state;<br>
}<br>
<br>
public Object saveTransientState(FacesContext context)<br>
{<br>
return transientState;<br>
}<br>
</blockquote>
<br>
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?<br>
<br>
<br>
<blockquote
cite="mid:AANLkTi=duVmXfOM3rn0Wn+vn_TZKTzLrtu2h=quZDkny@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
EB> If the rolled up state saved during the call to
markInitialState() is<br>
EB> not null or empty,<br>
<br>
AS> Since this is the "initial rolled up state" - ie. the full state,<br>
AS> wouldn't this be non-null?<br>
<br>
I'm not sure of the answer to this one. Leonardo?<br>
<br>
</blockquote>
<div><br>
In theory, if UIData.markInitialState is called correctly, the "initial
rolled up<br>
state" is not null, so the check could be skipped. In the first
versions of the<br>
algorithm, the check was used to fallback to the previous algorithm,
but now<br>
this is done using a check for the property (
isPreserveRowComponentState() ).<br>
</div>
</div>
</blockquote>
<br>
Hrm... so seems like this will never be null.<br>
<br>
<blockquote
cite="mid:AANLkTi=duVmXfOM3rn0Wn+vn_TZKTzLrtu2h=quZDkny@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
EB> If the per-row state saved in step a. above is null, traverse the<br>
EB> children and restore the child state using the initial rolled up
state.<br>
<br>
AS> Does the "per-row state saved in step a. above" refer to the
stave saved<br>
AS> for the row that we are leaving? Or for the state that we saved
the<br>
AS> last time that visited the current row?<br>
<br>
AS> That is, if we are on row 1 and setRowIndex(2) is called, does
the<br>
AS> "per-row state saved in step a" mean the state produced for row
1 before<br>
AS> we adjusted the index, or the state that we previously saved
away the<br>
AS> last time we visited row 2?<br>
<br>
AS> This is important to specify clearly.<br>
<br>
It's the state before we adjusted the row index. I've clarified that.<br>
<br>
EB> If the per-row state saved in step a. above is not null,
traverse the<br>
EB> children and restore the child state using the state saved
during step<br>
EB> a., using the initial rolled up state only as a backup in the
case<br>
EB> that per-row state is not available.<br>
<br>
AS> Okay, so I guess that "per-row state saved in step a. above"
refers to<br>
AS> the previously saved state for the current row.<br>
<br>
No, it's the state before we adjusted the row index. Leonardo, it is<br>
vitally important that you and Andy and I agree on this.<br>
<br>
</blockquote>
<div><br>
The "per-row state saved in step a. above" in other words means the
"delta"<br>
state from the row that will be changed. If we are on row 1, to change
to row 2<br>
we need to restore the state of row 2, and check if it is null or not,
If it is null<br>
we use only the initial rolled up state, but if is not, we use both the
initial<br>
rolled up state and the "per-row state saved".<br>
</div>
</div>
</blockquote>
<br>
This makes sense. Any time we being operating on a new row, we need to:<br>
<br>
1. Restore it to the initial state using the rolled up (full) state
that we saved during markInitialState(). And...<br>
2. Apply delta state, if there is any.<br>
<br>
Ed, this means that this clarification:<br>
<br>
<blockquote type="cite">It's the state before we adjusted the row
index. I've clarified that.<br>
</blockquote>
<br>
Is incorrect and needs to be re-clarified. :-)<br>
<br>
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.<br>
<br>
<blockquote
cite="mid:AANLkTi=duVmXfOM3rn0Wn+vn_TZKTzLrtu2h=quZDkny@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote"
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">AS>
However, since this is delta state, before we apply this isn't it<br>
AS> necessary to first restore the components to their initial state
by<br>
AS> restoring the full initial saved state? If we don't restore the<br>
AS> components to their initial state before applying the delta
state,<br>
AS> won't we run the risk that state from the previous row might
bleed<br>
AS> over into the current row? (In the case where the state from the<br>
AS> previous from was null, this won't be an issue.)<br>
<br>
AS> Also, at some point is it necessary to tell the StateHelper that
it<br>
AS> needs to clear out any previously saved deltas so that it can
start<br>
AS> tracking deltas for the currently active row? Or does that
happen<br>
AS> implicitly at some point, eg. when we restore the row state?<br>
</blockquote>
</div>
</blockquote>
<br>
<br>
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:<br>
<br>
<blockquote type="cite"> component.clearInitialState();<br>
if (childInitialState != null)<br>
{<br>
component.restoreState(facesContext,
childInitialState);<br>
component.markInitialState();<br>
component.restoreState(facesContext, childState);<br>
}<br>
</blockquote>
<br>
This seems reasonable.<br>
<br>
<blockquote
cite="mid:AANLkTi=duVmXfOM3rn0Wn+vn_TZKTzLrtu2h=quZDkny@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">EB>
If the per-row state saved in step b. above is null, traverse the<br>
EB> children and restore the transient state by passing null to each<br>
EB> child's<br>
EB>
UIComponent.restoreTransientState(javax.faces.context.FacesContext,<br>
EB> java.lang.Object) method<br>
EB><br>
EB> If the per-row state saved in step b. above is not null,
traverse the<br>
EB> children and restore the transient state from the state saved in
step<br>
EB> b. above, calling<br>
EB>
UIComponent.restoreTransientState(javax.faces.context.FacesContext,<br>
EB> java.lang.Object) on each child, and passing the appropriate
state.<br>
<br>
AS> Sounds like we are doing the same thing whether or not the
transient<br>
AS> state is null, so perhaps we can simplify the wording.<br>
<br>
Perhaps, but I'm concerned with getting it wrong. The current text was<br>
approved by Leonardo so I'm inclined to go with it.<br>
<br>
</blockquote>
<div><br>
In few words, it just say how to restore the "transient state". It is
possible in this<br>
part I just forget to simplify things. <br>
<br>
The time I wrote that part I was thinking if there was some kind of
manipulation <br>
on transient state properties before build view time, those values
should be <br>
"propagated" to child rows, but the intention of this state is to have
a very <br>
short life time (vdl build time or render time). If a value is
necessary <br>
to be stored more than the current request, use StateHelper. After
thinking about<br>
it carefully, the conclusion was that is not necessary, so we can just
assume a<br>
"null" or "empty" state for the transient map.<br>
</div>
</div>
</blockquote>
<br>
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.<br>
<blockquote
cite="mid:AANLkTi=duVmXfOM3rn0Wn+vn_TZKTzLrtu2h=quZDkny@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote"
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">AS>
Few other questions:<br>
<br>
AS> 1. Leonardo raised an issue regarding the timing of when<br>
AS> markInitialState() is called - ie. that markInitialState() needs
to be<br>
AS> called on the parent before the children - otherwise UIData
won't be<br>
AS> able to capture the full initial state of its children. How did
we<br>
AS> solve this problem? Are there spec changes relating to this?
Did we<br>
AS> find a way to do this that doesn't require introducing yet
another full<br>
AS> tree traversal?<br>
<br>
I don't know.<br>
<br>
</blockquote>
<div><br>
</div>
</div>
</blockquote>
<br>
I see that we have added an extra traversal, in
FaceletViewHandlingStrategy:<br>
<br>
<blockquote type="cite"> private void markInitialState(final
UIComponent component)<br>
{<br>
component.markInitialState();<br>
for (Iterator<UIComponent> it =
component.getFacetsAndChildren() ; it.hasNext() ; ) {<br>
UIComponent child = it.next();<br>
if (!child.isTransient()) {<br>
markInitialState(child);<br>
}<br>
}<br>
} <br>
</blockquote>
<br>
<br>
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.<br>
<br>
<br>
<br>
<blockquote
cite="mid:AANLkTi=duVmXfOM3rn0Wn+vn_TZKTzLrtu2h=quZDkny@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div>It is possible to prevent the full tree traversal, but to do
that it is necessary to<br>
change all code that uses PostAddToViewEvent to relocate components,
and that<br>
means refactor composite component solution as is. This was done on
MyFaces,<br>
so there cc:insertChildren and cc:insertFacet does not use a Listener
attached to<br>
that event there, but Mojarra uses it.<br>
<br>
Anyway, users could add components on PostAddToViewEvent to the tree,
so <br>
in theory the call to markInitialState() should be done after that
event is handled.<br>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:AANLkTi=duVmXfOM3rn0Wn+vn_TZKTzLrtu2h=quZDkny@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote"
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">AS>
2. Regarding the name of the new property...<br>
AS> is/setPreserveRowComponentState() is a bit of a mouthful. Could
we<br>
AS> maybe shorten this to is/setRowStatePreserved()?<br>
<br>
I like that a lot better.<br>
<br>
AS> 3. Are there cases where it might be useful to enable per-row
transient<br>
AS> state saving without also enabling non-transient state saving
(which<br>
AS> seems more expensive)? I wonder whether it should be possible
to enable<br>
AS> these independently, in which case we may want to consider using
an enum<br>
AS> property instead of a boolean. Also wondering whether transient
row<br>
AS> state saving should just be on by default or possibly always on.<br>
<br>
Leonardo?<br>
<br>
</blockquote>
<div><br>
It is possible, but shouldn't that condition just depends on the
component child itself ?.<br>
For example, UIForm has submitted property that in the future will be
stored in transient<br>
map, so this component requires transient state be always on.<br>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
Andy<br>
<br>
[1]
<a class="moz-txt-link-freetext" href="http://myfaces.apache.org/trinidad/devguide/configuration.html#org.apache.myfaces.trinidad.CACHE_VIEW_ROOT">http://myfaces.apache.org/trinidad/devguide/configuration.html#org.apache.myfaces.trinidad.CACHE_VIEW_ROOT</a><br>
<br>
<br>
</body>
</html>