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 <[email protected]> > To: MyFaces Development <[email protected]> > 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 <[email protected]> >> To: MyFaces Development <[email protected]>; Mark Struberg > <[email protected]> >> Cc: >> Sent: Thursday, November 15, 2012 6:20 PM >> Subject: Re: StateSaving review >> >> Hi >> >> 2012/11/15 Mark Struberg <[email protected]>: >>> 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 >> >
