Hi, On 12/27/06, Thomas Mueller <[EMAIL PROTECTED]> wrote:
So far I thought 'values' is quite a simple concept, and with some exceptions (for example the problems with streams) I thought it should be possible to implement it in a simple way. To tell you the truth, I don't understand the class diagram. For me, it is very complex, I'm confused. For example, I don't understand why you made the distinction between initial and committed values?
I'm using the State pattern to localize the Value state transitions to the Generic and Initial value classes. The current approach keeps the state in an explicit BaseValue.state member value and requires explicit setValueConsumed() calls in subclasses to keep the state updated. The proposed implementation simplifies this by removing all state handling form the committed value classes, leaving only the responsibilities of value representation and conversion to the committed values.
Also I don't understand what GenericValue is for (specially the hashCode method of this class seems strange, it looks like it is very inefficient because it is always creates a String and doesn't cache the hash value).
GenericValue implements the context part of the State pattern. It is essentially a wrapper around the GenericValue.value member variable that points to the current Value state. The state can either be an "initial" value (in case a value getter has not yet been called) or a "committed" value (in case a getter has already been called). The difference between those states is that an "initial" value can still be converted from stream to non-stream or vice versa, but a "committed" value can not. Caching the hash value would of course be possible if better performance is required, but I don't think Values are used that much as hash keys, so I think that simplicity is more important in this case. The getString() call is required in hashCode() to achieve proper state behaviour and to comply with the equals() contract.
Other things that are inefficient are CommittedStringValue.getXYZ() because those methods always create Parser objects. But I just don't understand the concept, I'm not saying it is 'wrong' (maybe we can guarantee that these functions are not used a lot).
I wanted to localize the value conversion mechanisms as much as possible to avoid duplicating code. Since an instance of the ValueParser class is essentially just a wrapper for the String to be parsed, I would expect a decent JVM to optimize away the allocation of the object (or to just allocate it as a temporary object on stack).
By the way, one concept I don't currently see in the value factory implementation is caching of commonly used values (except for CommittedTrueValue / CommittedFalseValue). In my experience, this improves the performance and saves a lot of memory because it is very common that a small set of values is used in many places. I could implement such a feature later on if it is not implemented yet.
Good point, and actually something that couldn't be possible without separating the "committed" value state. It would be nice to see some performance numbers before doing that though, since a generational garbage collector allows amazingly good performance with large numbers of small objects.
SerializableInputStream: if I understand it correctly, then the old implementation allowed very large (that don't fit in memory) binary values, and the new implementation does not? Please correct me if I'm wrong.
You're wrong. The whole stream is consumed in memory only when a Value gets serialized, something that the current implementation doesn't even support. In any case both implementations are even required to consume the entire stream if getString() is called. In normal circumstances (i.e. only getStream() called on binary values), the proposed implementation simply passes along the InputStream received in ValueFactory.createValue(InputStream) just like the current implementation does. Note however that simply doing that in ValueFactory.createValue(InputStream) is most likely incorrect as the client that makes the call has currently no way of knowing when or who should close the stream to reclaim any resources (file handles, etc.) associated with the stream. The proposed Value implementation does not attempt to solve this isssue. PS. I should probably present the proposed changes as a sequence of incremental changes than as a single revolutionary patch. BR, Jukka Zitting
