So your vote is: 1. Go back to the old way, just one class. 2. The viewId is not important --> get rid of it. 3. Split ServerSideStateCacheImpl into many classes
Let's try to be concrete, so we can get to a constructive conclusion. I think both positions are clear, I don't want to waste my time in futile discussions, it is pointless. I respect and appreciate your arguments, but let the people interested take a position, otherwise this will go to nowhere. This is not like politics, we need to take action and the sooner the better. 2012/11/15 Mark Struberg <[email protected]>: > >> 1. Small key size used to store views into session vs go back to the old way. > > Take a safe approach. Adding just the hashCode doesn't make it more safe. > Either the full viewId or get rid of it completely if not needed. > > >> 2. use the hashCode of the viewId vs store the full viewId and do an >> equals comparison vs remove it from the key. > IF the viewId is important than we must do the equals on the viewId in > addition. > > >> 3. Split ServerSideStateCacheImpl into many classes vs keep the code as is. > Split into own package. It's really not maintainable. Btw, I like the old > StateManagerImpl name better than the 'Cache'. It is much more to the point > imo. > > > > Now for the details: > Most of your arguments are unrelated. E.g. Serializing a String is exactly as > cheap as serializing a byte[]. There is really zero difference. As a last comment, the int stored in replacement of the viewId using hashCode() method is just 4 bytes vs a String with the full viewId, which has a lenght of more than 10 or 20 characters, and that could be 44 bytes, maybe more? If the session is not serialized the effect is similar. There is a difference and is not zero or near zero. It is enough significant to take some time and try to minimize it. Does it helps the hashCode of viewId? yes, because with only 4 bytes we can be sure with a very high probability that the state can be applied to the view. It is strictly necessary? no. To show it in practice this is the generated markup for javax.faces.ViewState right now using the default encryption algorithm: <input type="hidden" name="javax.faces.ViewState" id="javax.faces.ViewState" value="rO0ABXQAATE=" /> This is what we had before (myfaces 2.1.7): <input type="hidden" name="javax.faces.ViewState" id="javax.faces.ViewState" value="rO0ABXVyABNbTGphdmEubGFuZy5PYmplY3Q7kM5YnxBzKWwCAAB4cAAAAAJ1cQB+AAAAAAACdAABMXB0AAsvaG9tZS54aHRtbA==" /> Which one is better? It is worth to do it? which one will be faster to encrypt? > Also the impls didn't change. It's now just much more complicated than > before. I would have expected to do the obvious improvements like getting rid > of the synchronized block by simply using an AtomicInteger as counter. THOSE > things bring performance. > >> Even if sounds a good idea to do it, the current design using private >> static classes >> ensures that it is not possible to create keys outside the algorithm >> and override >> the values inside the internal map that holds the view state. > This is preventing exactly nothing. UIViewRoot has a public interface to > change that. Any JavaScript code on the client can change it. > You can prevent both if a SM is in place and you can perfectly invoke both if > not. > It only makes it harder to maintain and extend the code. > > > LieGrue, > strub > > > > > ----- Original Message ----- >> From: Leonardo Uribe <[email protected]> >> To: MyFaces Development <[email protected]> >> Cc: >> Sent: Thursday, November 15, 2012 2:17 AM >> Subject: [VOTE] Design of MyFaces server side state saving algorithm and >> related considerations >> >> Hi >> >> Recently in >> >> https://issues.apache.org/jira/browse/MYFACES-3638 >> >> We have found some problems related to how ServerSideStateCacheImpl should >> work. In few words, the code in that class deals with all logic >> related to server >> side state saving, so it is a critical point in MyFaces Core. >> >> We need some help to get this out, so a vote over this considerations >> is necessary. >> It is a long story, so I divided the mail into sections, so if you >> already know the >> background, you can skip to the important parts. >> >> --- BACKGROUND --- >> >> In 1.1.x and 1.2.x, this code was in this location: >> >> http://svn.apache.org/repos/asf/myfaces/core/branches/1.2.x/impl/src/main/java/org/apache/myfaces/application/jsp/JspStateManagerImpl.java >> >> The idea is when a view needs to be saved, a key is created >> (SerializedViewKey) >> using a session scope counter and the full viewId. This key is later >> serialized, >> encrypted, tampered (add a mac code), and base64 encoded to be stored in >> javax.faces.ViewState hidden field. Also, the key is used in a internal map >> to >> store the calculated view state that will be used later to restore the view. >> >> In MYFACES-3117, we saw the need to align MyFaces state saving >> implementation with the one in the spec, and fix some problems related to the >> implementation of ResponseStateManager. Some changes were done to >> isolate responsibilities, and the code in JspStateManagerImpl was moved to >> ServerSideStateCacheImpl. >> >> In MYFACES-3563, another round of improvements were done (changes done >> in 2.1.9 / 2.0.15), with the intention of remove some old code that >> does not have >> sense anymore with the new implementation and improve server side state >> saving, trying to mimize the overhead associated with store a view. >> >> One of the changes done was this: >> >> https://issues.apache.org/jira/browse/MYFACES-3568 >> >> [perf] use viewId hashCode() instead full viewId on server side state saving >> SerializedViewKey >> >> The justification of this improvement is this: >> >> "... In SerializedViewKey, the current code uses a counter and the full >> viewId as key to retrieve or save the state into session. >> >> But store the full viewId is not really necessary. The counter itself gives >> uniqueness inside the session, and the viewId is just a way to check if >> the state to restore is the right one. In other words, given the probability >> of found two valid viewIds in an application with the same hashCode is >> astronomical and the fact that there is a counter that identify in an unique >> way a view state session token, it is reasonable to store into the state >> only the hashCode, and when the view is restored compare the viewId >> hashCode with the stored one into the state. >> >> This change will reduce the size required by SerializedViewKey. ..." >> >> Originally, the idea of use both a viewId and a counter to derive a key was >> done because the old algorithm relies on the viewId to ensure uniqueness >> of the key. That's how is working now in 1.2.x / 1.1.x and it is working >> just fine. >> >> But based on the improvements done in MYFACES-3568 and MYFACES-3117 >> it was realized that only the counter is required to ensure key uniqueness, >> and the viewId comparison is not necessary. For example, Mojarra allows >> to use a counter or a random number to generate valid keys (see >> com.sun.faces.generateUniqueServerStateIds web config parameter). >> >> --- DISCUSSION POINTS --- >> >> In MYFACES-3638, some changes were done, trying to simplify the code, but >> ignoring some considerations already accepted. In few words the controversial >> points are: >> >> 1. In MYFACES-3568, the following private internal classes extending from >> an abstract class called SerializedViewKey: >> >> - IntIntSerializedViewKey >> - IntByteArraySerializedViewKey >> - ReferenceSerializedViewKey >> >> The idea is serialize an Object is more expensive than serialize a >> primitive type. >> So, using IntIntSerializedViewKey and IntByteArraySerializedViewKey in >> production stage will lead to a smaller session size compared to use the old >> implementation of SerializedViewKey, with requires to store the full viewId. >> >> The advantages of this improvement are: >> >> - Small session size >> - Since the key is small it will be faster to encrypt and encode it. >> >> See this link for details about this: >> >> http://www.cs.virginia.edu/kim/publicity/pldi09tutorials/memory-efficient-java-tutorial.pdf >> >> but the claimed disadvantage is that three classes for do the same that can >> be >> done just once, just increase complexity without need. >> >> 2. There is still doubts about if the full viewId should be stored in >> the key or not. The >> evidence suggest that it is not necessary, but if that so, it is worth >> to store the hashCode >> of the viewId? In theory, the probability of have two real viewIds >> with the same hashCode >> is astronomical, so even if it is not the same as an equality >> comparison, it helps to >> decide if a state can be applied to a view or not. >> >> 3. Since there are too many classes inside ServerSideStateCacheImpl, >> it is better >> practice to put those classes outside the class, to have a better >> layout of the code. >> Even if sounds a good idea to do it, the current design using private >> static classes >> ensures that it is not possible to create keys outside the algorithm >> and override >> the values inside the internal map that holds the view state. For example, >> with >> Java 2 security it is possible to ensure that code cannot be executed >> using reflection >> and so on. Putting the classes outside the class makes harder to control >> that. >> >> Note that myfaces code already uses an strict checkstyle policy. >> >> -- VOTE -- >> >> Please vote for each point: >> >> 1. Small key size used to store views into session vs go back to the old way. >> 2. use the hashCode of the viewId vs store the full viewId and do an >> equals comparison vs remove it from the key. >> 3. Split ServerSideStateCacheImpl into many classes vs keep the code as is. >> >> Please note that in case we can't get to an agreement, according to >> the rules, since the >> changes in MYFACES-3563 were committed long time ago without any objections, >> lazy consensus applies[1], and the changes proposed recently needs to >> be reverted. >> >> regards, >> >> Leonardo Uribe >> >> [1] http://community.apache.org/committers/lazyConsensus.html >>
