harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexei Fedotov" <alexei.fedo...@gmail.com>
Subject Re: [classlib][luni][performance] java.io.ObjectStream* cleanup (Re: [general][performance] Serialization performance optimization results)
Date Thu, 05 Jun 2008 21:24:20 GMT
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 <aleksey.shipilev@gmail.com>
wrote:

> 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
> >
>



-- 
With best regards,
Alexei

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message