> You did more than that. You removed the following classes: > > IntIntSerializedViewKey > IntByteArraySerializedViewKey > ReferenceSerializedViewKey
Those have been almost 1:1 clones of other classes. Go check SerializedViewKey, it contains all the merged functionality. Without any performance overhead. Look at the code first and then you can complain. You revert code done by other people without even looking at the changes it seems. > and on top of that you removed JspStateManagerImpl, which is a class > that is there for compatibility with old state managers that relies on > MyFaces 1.2.x. This is not JSF-1.x since a long time. APIs in the StateManager got changed for AJAX support, etc. But I'm all fine with going back to the JspStateManagerImpl again as I already said. Btw, may I quote your answer to my question if I can delete it? LU> "Remove that code is a valid option." LieGrue, strub ----- Original Message ----- > From: Leonardo Uribe <[email protected]> > To: MyFaces Development <[email protected]>; Mark Struberg > <[email protected]> > Cc: > Sent: Thursday, November 15, 2012 10:20 PM > Subject: Re: StateSaving review > > Hi > > 2012/11/15 Mark Struberg <[email protected]>: >> 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. >> > > You did more than that. You removed the following classes: > > IntIntSerializedViewKey > IntByteArraySerializedViewKey > ReferenceSerializedViewKey > > also you created this one (it was an abstract class before) > > SerializedViewKey > > and on top of that you removed JspStateManagerImpl, which is a class > that is there for compatibility with old state managers that relies on > MyFaces 1.2.x. Obviously you have not considered those cases, but I have > done that long time ago. > > Without ask, without propose a patch, without let the people who care about > to participate. Just do it and let others do the clean up. > > Mark, I have came here and proposed to you many possible alternatives > and you're letting me without choices. I beg you to move your changes to > 2.1.x-client-window branch, and when we have certain about which changes > needs to be done, backport them to 2.1.x. In just one step, without this > pain and stress you are causing me. Please. The reason why it was created > 2.1.x-client-window was precisely to try changes for client window / state > manager and flash scope. > > In this moment I can't do any release. You have tied my hands. I'm > trying to be > kind, please. > >> 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. >> > > It is a work in progress, I already told you many times. I know the > abstraction > is not the best, but it works, and has helped a lot to me to understand how > this algorithm should work. It is like a worm that became chrysalis and in the > future will become a butterfly. "... the morphing step is not nice > ..." > > regards, > > Leonardo > >> 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 >>>>>>>> >>>>>>> >>>>> >>> >
