myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Struberg <strub...@yahoo.de>
Subject Re: StateSaving review
Date Thu, 15 Nov 2012 17:50:37 GMT
Btw, I still do not see where the trick for storing a separate state list for each browser
tab is hidden.
That was the goal of all the refactoring initially. Where is that done?

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/broken 
> algorithms.
> 
> 
> Proposal: Get rid of all that KeyFactory stuff again and have one GOOD 
> 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 always 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
>> 
> 

Mime
View raw message