Hello Aleksey, Thank you for a diligent cleanup of the object stream code. If you are looking for a sort of review for four JIRA issues, please find my comments below concerning HARMONY-5847 [1].
[1] https://issues.apache.org/jira/browse/HARMONY-5847 * This is not transparent why you refer to the next handle differently in the newly developed code: + Integer newHandle = nextHandle(); + int newHandle = nextHandle(); * Why you call to ObjectStreamClass.lookupStreamClass(superclass) from the outside of readObjectNoData() and use three parameters instead of two? - readObjectNoData(object, superclass); + readObjectNoData(object, superclass, ObjectStreamClass.lookupStreamClass(superclass)); * I like TODO comments in the patch. From the other side some comments look pretty mysterious: + // TODO: Here is the opportunity for enhancement + // We can implement it through fast-path, without + // setting up the context with public API Both "it" and "context" are not defined. Also tabs make the code look strangely aligned. * Why you use an assignment instead of just returning updateReference(object, unshared)? + int handle = updateReference(object, unshared); return handle; * It seems that inlining updateReference(object, unshared) in all three locations would result in more compact and readable code because the calls are always preceded with if (unshared) {}. Also it concerns me that the previous code does some job for both cases. /** + * Updates the references table with new handle + * + * @param obj + * Non-null object being updated + * @param unshared + * whether the handle is unshared + */ + private int updateReference(Object object, boolean unshared) { + int handle = nextHandle(); + + if (!unshared) { + objectsWritten.put(object, handle); + } + + return handle; + } * Why cannot you set up the following properties in the constructor instead of checking arePropertiesResolved on each access? + /** + * Cached class properties + */ + private transient boolean isSerializable; + private transient boolean isExternalizable; + private transient boolean isProxy; + private transient boolean isEnum; * I believe the dot should be placed at <code>java.langClass</code>. On Fri, May 30, 2008 at 3:39 PM, Aleksey Shipilev <[EMAIL PROTECTED]> wrote: > Hi, > > Here is the scrub of the serialization performance improvements we > have today. I had used SPECjvm2008:serial as reference benchmark, > running it on 8-core Xeon (E5440/2.86Ghz/HTN) / 24 Gb DDR2-667 / > Windows 2003 EE SP1. All measurements were done in 5 iterations, 240 > secs per one iteration, max result was used as score. Scores are > ops/min, the more the better. All JVMs were run in "-server -Xmx512M > -Xms512M" mode. Measurement deviations are within 3%. > > Baseline measurements: > Sun 1.6.0_05 -server 145.0 > Harmony r641838 -server 29.4 > > So, Harmony performance was only 20% of RI on serialization workload. > > The improvements: > (JIRA#, Harmony score, boost relative to baseline, status relative to RI.) > > -- already in trunk ----- > HARMONY-5635, 33.1, 13%, 23% > HARMONY-5634, 35.4, 20%, 24% > HARMONY-5640, 36.2, 23%, 25% > HARMONY-5633, 68.7, 134%, 47% > HARMONY-5735, 69.7, 137%, 48% > HARMONY-5722, 80.4, 174%, 55% > HARMONY-5756, 83.2, 183%, 57% > HARMONY-5770, 85.1, 190%, 59% > > -- ready for review and commit ------ > HARMONY-5761, 88.4, 201%, 61% > HARMONY-5829, 95.7, 226%, 66% > HARMONY-5847, 110.7, 277%, 76% > HARMONY-5771, 136.5, 365%, 94% > > -- need debug and review (estimated boosts) ----- > HARMONY-5713, 150.2, 411%, >>>104%<<< > > The dry results of these measurements are: > 1. After the committing rest of the _ready_ issues we will be close > to Sun's performance. Nathan is working on HARMONY-5829 and > HARMONY-5847 now, but HARMONY-5761 (WeakHashMap improvements) and > HARMONY-5771 (IdentityHashMap improvements) are still orphaned. Can > someone take them? > > 2. After the completion of HARMONY-5713 we will beat the RI on > serialization benchmark. > > Thanks, > Aleksey. > -- With best regards, Alexei
