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
