Thanks, Aleksey, for patch improvements! It encourages me as a reviewer that you find some of my comments useful. On Thu, Jun 5, 2008 at 8:42 PM, Aleksey Shipilev wrote: > Hi, Alexei! > > Thanks for the review, here are the comments. > > On Fri, May 30, 2008 at 6:09 PM, Alexei Fedotov > wrote: > > * This is not transparent why you refer to the next handle differently > > in the newly developed code: > > + Integer newHandle = nextHandle(); > > + int newHandle = nextHandle(); > I'm relying here on compiler ability to box/unbox primitive values > rather than doing that by hand. The initial approach for having > Integer is to distinguish "null" handle. I had changed it back to > primitive where such "null" handle is not required. Of course, should > I place the handle in HashMap and the boxing will > occur, but here I reserve simpler task for the compiler to scalarize > this boxed Integer instance. > > > * 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)); > This is just for the conformity reasons. I had propagated classDesc > everywhere and on readObjectNoData there was no ready superclassDesc, > so I had to look it up. I think that we should leave this change > intact thus further refactoring (if any) will use the same interface > and probably save one of the lookups. > > > * 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. > Thanks, I had updated the patch. > > > * 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. > Inlined in new version. > > > * Why cannot you set up the following properties in the constructor > > instead of checking arePropertiesResolved on each access? > My investigation shows that such lazy initialization brings much more > performance improvement than initialization in constructor. > > Thanks, > Aleksey. > > > > * I believe the dot should be placed at java.langClass. > > > > > > On Fri, May 30, 2008 at 3:39 PM, Aleksey Shipilev > > 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 > > > -- With best regards, Alexei