Hi, Alexei!
Thanks for the review, here are the comments.
On Fri, May 30, 2008 at 6:09 PM, Alexei Fedotov
<[EMAIL PROTECTED]> 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<Object,Integer> 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 <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
>
