harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aleksey Shipilev" <aleksey.shipi...@gmail.com>
Subject [classlib][luni][performance] java.io.ObjectStream* cleanup (Re: [general][performance] Serialization performance optimization results)
Date Thu, 05 Jun 2008 16:42:54 GMT
Hi, Alexei!

Thanks for the review, here are the comments.

On Fri, May 30, 2008 at 6:09 PM, Alexei Fedotov
<alexei.fedotov@gmail.com> 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
> <aleksey.shipilev@gmail.com> 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
>

Mime
View raw message