You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Aleksey Shipilev <al...@gmail.com> on 2008/06/05 18:42:54 UTC
[classlib][luni][performance] java.io.ObjectStream* cleanup (Re: [general][performance] Serialization performance optimization results)
Hi, Alexei!
Thanks for the review, here are the comments.
On Fri, May 30, 2008 at 6:09 PM, Alexei Fedotov
<al...@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
> <al...@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
>
Re: [classlib][luni][performance] java.io.ObjectStream* cleanup (Re: [general][performance] Serialization performance optimization results)
Posted by Alexei Fedotov <al...@gmail.com>.
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 <al...@gmail.com>
wrote:
> Hi, Alexei!
>
> Thanks for the review, here are the comments.
>
> On Fri, May 30, 2008 at 6:09 PM, Alexei Fedotov
> <al...@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
> > <al...@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
> >
>
--
With best regards,
Alexei