Btw, the implementation shares state via the context map using RendererUtils.SEQUENCE_PARAM all over the place. Please check the usage of RendererUtils.SEQUENCE_PARAM!
Again: I think you splitted this up way too fine grained and now you need a way to hand over information via bypassing your own internal api. That doesn't work out imo. To me all this is clearly a sign that we went too far with the abstraction and we must go back one step (or two). LieGrue, strub ----- Original Message ----- > From: Mark Struberg <[email protected]> > To: MyFaces Development <[email protected]> > Cc: > Sent: Thursday, November 15, 2012 9:46 PM > Subject: Re: StateSaving review > > 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 >>>>>>> >>>>>> >>>> >> >
