Mailing-List: contact dev-help@myfaces.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "MyFaces Development" <dev@myfaces.apache.org>
Received-SPF: pass (nike.apache.org: domain of lu4242@gmail.com designates
 209.85.160.53 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <1353010534.36957.YahooMailNeo@web28903.mail.ir2.yahoo.com>
References: 
 <CABujnVcFX__TpVsOGT4TX8GfpdECKohJc+SYMSd147us064S1Q@mail.gmail.com>
	<1352957863.64125.YahooMailNeo@web28905.mail.ir2.yahoo.com>
	<CAM1yOjbk0eh4hCSusCC0xkvcmbKv9qLK5S5A0W4E_W=edZrgTQ@mail.gmail.com>
	<1352993336.93244.YahooMailNeo@web28906.mail.ir2.yahoo.com>
	<CABujnVdBX0nbbB5uJk9SAg5_XEW7H7+r-OqG1bto1A+GSqi-3w@mail.gmail.com>
	<1353000723.47942.YahooMailNeo@web28906.mail.ir2.yahoo.com>
	<1353001837.70696.YahooMailNeo@web28906.mail.ir2.yahoo.com>
	<CABujnVdd2KJ7bGxP2Tib34sb=qs0WsB_u9-LG3JUpKZwAccvLA@mail.gmail.com>
	<1353010534.36957.YahooMailNeo@web28903.mail.ir2.yahoo.com>
Date: Thu, 15 Nov 2012 15:30:06 -0500
Message-ID: 
 <CABujnVfjEojmQOG5_2OMCfvO12GRDfi6wk5KnGHoNs_6Cw1J4w@mail.gmail.com>
Subject: Re: StateSaving review
From: Leonardo Uribe <lu4242@gmail.com>
To: MyFaces Development <dev@myfaces.apache.org>,
 Mark Struberg <struberg@yahoo.de>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

Hi

2012/11/15 Mark Struberg <struberg@yahoo.de>:
> Leo, I'm ok with a revert. But not to your version but to the original Js=
pStateManagerImpl!
>
> Really, this currently feels so overcomplicated without giving much benef=
it. It would have been perfectly enough to remove the viewId String and rep=
lace it with a XORShift random value if you don't trust the sequencer.
>
> I know one goal was to abstract out the state between client and server. =
But that doesn't mean that we end up with Object, Object and do hardcoded u=
pcasts as done right now in the code. This is no improvement over the origi=
nal code. The windowId change would have been 30 LOC btw.
>

It is fine if you want to change the implementation to make something
better. What's not fine, is that you want to work in 2.1.x branch
directly, that is right now in maintenance stage. So, you are doing
all kind of refactorings, and that makes me crazy. The last release
was done on 23/Sep/12 and I'm doing 1 release each 2 months. Your
changes will take longer and I need time to review them, because I
will not do a release over changes that I have not seen.

My idea is start a release process next week. Why don't you take your
time?, instead you try to do changes without any clear direction.

In this moments, we are starting the necessary steps to work on 2.2.x.
You should be working in 2.1.x-client-window branch !!!!!

regards,

Leonardo Uribe

> LieGrue,
> strub
>
>
>
>
> ----- Original Message -----
>> From: Leonardo Uribe <lu4242@gmail.com>
>> To: MyFaces Development <dev@myfaces.apache.org>; Mark Struberg <struber=
g@yahoo.de>
>> Cc:
>> Sent: Thursday, November 15, 2012 8:44 PM
>> Subject: Re: StateSaving review
>>
>> Hi
>>
>> 2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>>  Btw, I still do not see where the trick for storing a separate state l=
ist
>> for each browser tab is hidden.
>>>  That was the goal of all the refactoring initially. Where is that done=
?
>>>
>>
>> Mark, months ago we created 2.1.x-client-window branch:
>>
>> http://svn.apache.org/repos/asf/myfaces/core/branches/2.1.x-client-windo=
w/
>>
>> This branch is the same as 2.1.x, but only with the changes proposed
>> long time ago for JSF 2.2 client window.
>>
>> It has what you need to have a state list for each browser tab.
>> The only thing you need to provide is a ClientWindow implementation
>> and that's it. The state saving code already has included the client-win=
dow
>> concept. I also fixed Flash scope to use client window.
>> It is also available in a maver repo:
>>
>> https://repository.apache.org/content/repositories/snapshots/org/apache/=
myfaces/core/myfaces-bundle/
>>
>> Let's make a deal. Move your changes to that branch, let me apply my fix
>> in trunk and do a release. When your changes are ready, we can discuss
>> them and backport from 2.1.x-client-window to trunk. Does that sound
>> good for you?
>>
>> regards,
>>
>> Leonardo Uribe
>>
>>
>>>  LieGrue,
>>>  strub
>>>
>>>
>>>
>>>  ----- Original Message -----
>>>>  From: Mark Struberg <struberg@yahoo.de>
>>>>  To: MyFaces Development <dev@myfaces.apache.org>
>>>>  Cc:
>>>>  Sent: Thursday, November 15, 2012 6:32 PM
>>>>  Subject: Re: StateSaving review
>>>>
>>>>  I did further reviews:
>>>>
>>>>  Currently the CounterKeyFactory needs some random to be unique
>> (according to
>>>>  Leo) and the RandomKeyFactory needs a counter to be unique.
>>>>
>>>>  Does that ring a bell? That stuff is completely useless!
>>>>
>>>>
>>>>  Foooooolks wake up, let's provide ONE factory which is waterproof.
>> That
>>>>  would be much better than having 15++ classes full of half baken/brok=
en
>>>>  algorithms.
>>>>
>>>>
>>>>  Proposal: Get rid of all that KeyFactory stuff again and have one GOO=
D
>>>>  algorithm: Counter + XORShift Random.
>>>>
>>>>  The KeyFactory stuff got only introduced in 2.1.9 and is crashing in
>> production.
>>>>
>>>>
>>>>  LieGrue,
>>>>  strub
>>>>
>>>>  PS: don't take it personal, but from looking at the code I see
>> clearly what
>>>>  happens if someone works on a stuff alone without reviews. This alway=
s
>> leads to
>>>>  deficits, regardless how good one is.
>>>>
>>>>
>>>>  ----- Original Message -----
>>>>>   From: Leonardo Uribe <lu4242@gmail.com>
>>>>>   To: MyFaces Development <dev@myfaces.apache.org>; Mark
>> Struberg
>>>>  <struberg@yahoo.de>
>>>>>   Cc:
>>>>>   Sent: Thursday, November 15, 2012 6:20 PM
>>>>>   Subject: Re: StateSaving review
>>>>>
>>>>>   Hi
>>>>>
>>>>>   2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>>>>>    I'm currently reviewing all this area. It seems that we
>> have quite
>>>>  some
>>>>>   stuff to improve.
>>>>>>
>>>>>>    a.) just a gut feeling yet, but my tummy tells me that we
>> have to
>>>>  review
>>>>>   our key generator/storage strategies. Too complicated or too badly
>>>>  documented.
>>>>>   At least they are not self describing.
>>>>>
>>>>>   It's a complex algorithm. No documentation so far, because it
>> is still
>>>>>   work in progress.
>>>>>
>>>>>>
>>>>>>    b.) candidate 1: CounterKeyFactory. If we like to prevent
>> reboot
>>>>  clashes
>>>>>   then we might add another int which contains a random value. Think
>> about
>>>>  having
>>>>>   a Server with a single page right now. Click on it a few times,
>> then
>>>>  restart
>>>>>   myfaces and issue a few requests to the same page and go back in
>> your
>>>>  browser
>>>>>   history. Proposal: instead of the viewId we should add a random
>> number.
>>>>>>
>>>>>
>>>>>   Since the counter is also stored into session, the last counter
>> values
>>>>>   is not lost.
>>>>>
>>>>>>    c.) a general one. We might introduce an own Random which
>> either uses
>>>>>   java.util.concurrent.ThreadLocalRandom for java 7++ or the old
>> Random impl.
>>>>>>     ThreadLocalRandom has a _much_ better performance on
>> servers! Or we
>>>>  just
>>>>>   use a simple XORShift which is surely good enough for most cases
>> and
>>>>  performs
>>>>>   like hell. The spreading of XORShift is better than the standard
>> Java
>>>>  algorithm
>>>>>   even ...
>>>>>>
>>>>>
>>>>>   It could work.
>>>>>
>>>>>>    d.) KeyFactory looks a bit overengineered. The return type is
>> either
>>>>>   Integer or byte [] but the encoded value is always represented as
>> String.
>>>>>>
>>>>>
>>>>>   The key is encrypted and base64 encoded, but that's done in
>>>>>   HtmlResponseStateManager.writeViewStateField. The change only has
>>>>>   sense if we move the responsibility of encrypt to
>>>>>   ServerSideStateCacheImpl/ClientSideStateCacheImpl. My opinion is
>> it is
>>>>>   better to keep it as is.
>>>>>
>>>>>>    e.) Instead of trashing the Session with setAttribute and
>> synchronized
>>>>
>>>>>   blocks we should rather store an AtomicInteger. This is perfectly
>> fine now
>>>>  as we
>>>>>   do not support java 1.4 any longer, right?
>>>>>>
>>>>>
>>>>>   The synchronized blocks are over the SerializedViewCollection,
>> which
>>>>>   is unique per session, so it does not suppose any overhead (tested
>>>>>   with YourKit profiler). But performance tests shows that the
>>>>>   additional calls to session map impose an small overhead (the
>> ideal is
>>>>>   minimize those calls). Since the counter is stored per session,
>> there
>>>>>   is no chance of key clashing. One option could be something like
>> the
>>>>>   trick used in Flash Scope. An AtomicLong from a random seed, but
>>>>>   anyway it is necessary to change the algorithm for check if there
>> is a
>>>>>   key clashing, generates another key.
>>>>>
>>>>>>    Just a few random ideas...
>>>>>>
>>>>>
>>>>>   MS>> another one:
>>>>>   MS>> All that stuff has nothing to do with the RenderKit and
>> should
>>>>  go
>>>>>   into org.apache.myfaces.application.viewstate.
>>>>>   MS>> wdyt?
>>>>>
>>>>>   That stuff is used by
>>>>>   org.apache.myfaces.renderkit.html.HtmlResponseStateManager, so in
>>>>>   theory is related to the renderkit (because ResponseStateManager
>> is
>>>>>   renderkit dependant), but it is an application scope concept.
>> Maybe
>>>>>   org.apache.myfaces.renderkit.application.viewstate. has more sense
>> to
>>>>>   me.
>>>>>
>>>>>   MS>> Also the following classes are related and should imo
>> get moved
>>>>>   to this new package:
>>>>>   MS>> * StateCache
>>>>>   MS>> * StateCacheFactory
>>>>>
>>>>>   Yes, a better name
>>>>>
>>>>>   MS>> * StateManagerImpl
>>>>>
>>>>>   No, StateManagerImpl deals with the logic related to calculate the
>>>>>   state to a view, StateCache deals with the storage of the view
>> (maybe
>>>>>   a better name is StateStorage, for example
>>>>>   ServerSideStateStorageImpl/ClientSideStateStorageImpl?
>>>>>
>>>>>   regards,
>>>>>
>>>>>   Leonardo
>>>>>
>>>>>>    LieGrue,
>>>>>>    strub
>>>>>
>>>>
>>