Leo, the stuff which currently got changed is nothing more than just your code. I only refactored it into an own package and moved the inner classes to toplevel to show up the complexity of the solution. There was no code change _yet_ other than the viewId. We can change back the viewId to the hash if you feel better. It makes no difference anyway.
In the long run this stuff needs _heavy_ cleanup. Again: Extracting out an abstraction over Server vs Client stateSaving is perfectly fine, but the code is now way more complicated than it ever has been. Premature abstraction is way worse than premature optimisation. LieGrue, strub ----- Original Message ----- > From: Leonardo Uribe <[email protected]> > To: MyFaces Development <[email protected]>; Mark Struberg > <[email protected]> > Cc: > Sent: Thursday, November 15, 2012 9:30 PM > Subject: Re: StateSaving review > > Hi > > 2012/11/15 Mark Struberg <[email protected]>: >> Leo, I'm ok with a revert. But not to your version but to the original > JspStateManagerImpl! >> >> Really, this currently feels so overcomplicated without giving much > benefit. It would have been perfectly enough to remove the viewId String and > replace 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 > upcasts as done right now in the code. This is no improvement over the > original > 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 <[email protected]> >>> To: MyFaces Development <[email protected]>; Mark Struberg > <[email protected]> >>> Cc: >>> Sent: Thursday, November 15, 2012 8:44 PM >>> Subject: Re: StateSaving review >>> >>> Hi >>> >>> 2012/11/15 Mark Struberg <[email protected]>: >>>> 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? >>>> >>> >>> Mark, months ago we created 2.1.x-client-window branch: >>> >>> > http://svn.apache.org/repos/asf/myfaces/core/branches/2.1.x-client-window/ >>> >>> 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-window >>> 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 <[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 >>>>>> >>>>> >>> >
