harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Cornwall" <andrew.pack...@gmail.com>
Subject Re: [jira] Commented: (HARMONY-5900) [classlib][pack200] CpBands.parseCpSignature(Ljava/io/InputStream;) is hot
Date Tue, 08 Jul 2008 17:03:46 GMT
I guess maybe Sian's patch wasn't meant to get search out, but just test if
the CPUTF8 signatures appeared in the right place? If so, they do - I've
just confirmed it. I think the signature code may be obsolete. Does this
mean we can get rid of it? Do we need to do the search (or Aleksey's
equvialent) if we don't need to distinguish different kinds of CPUTF8?

Aleksey, I can confirm your patch provides a performance boost and does not
appear to change any behaviour in unpacking.

    Andrew Jr.

On Tue, Jul 8, 2008 at 9:48 AM, Aleksey Shipilev <aleksey.shipilev@gmail.com>
wrote:

> Andrew,
>
> There's a hint - search() called for several distinct String[] arrays.
> Please see my patch, it eliminates search() at all, there you can find
> the search() consumers.
>
> Thanks,
> Aleksey.
>
> On Tue, Jul 8, 2008 at 8:43 PM, Andrew Cornwall
> <andrew.pack200@gmail.com> wrote:
> > I applied Sian's patch from HARMONY-5900, but that doesn't appear to
> change
> > the runtime characteristics very much. In particular, it appears that
> > CpBands.search is still being called - did I miss something in the patch?
> >
> >    Andrew Jr.
> >
> > On Tue, Jul 8, 2008 at 9:25 AM, Andrew Cornwall <
> andrew.pack200@gmail.com>
> > wrote:
> >
> >> Sian, are we still using the DOMAIN_* code for ordering? I thought you'd
> >> changed the code to use the ordering defined in the pack200 archive? If
> so,
> >> we might be able to get rid of all the DOMAIN_ code.
> >>
> >>     Andrew Jr.
> >>
> >>
> >>
> >> On Tue, Jul 8, 2008 at 4:05 AM, Aleksey Shipilev <
> >> aleksey.shipilev@gmail.com> wrote:
> >>
> >>> Ah! IMO, that's ok in terms of performance.
> >>> Anyway, we can implement Map storage for CpSignatures and have no
> >>> degradation at all.
> >>>
> >>> Let's wait for Andrew.
> >>>
> >>> Thanks,
> >>> Aleksey.
> >>>
> >>> On Tue, Jul 8, 2008 at 3:00 PM, Sian January <
> sianjanuary@googlemail.com>
> >>> wrote:
> >>> > I posted a suggested alternative on the JIRA, which doesn't do this
> >>> search
> >>> > at all and just uses the same objects for CpSignatures that were
> >>> transmitted
> >>> > in CpUtf8 if they exist.  The downside of this is that
> >>> > CpBands.cpSignatureValue(..)
> >>> > will always be searching a much larger map, so I'm not sure if there
> are
> >>> > performance implications from that.  I'm also not 100% sure that we
> >>> don't
> >>> > need the code I've taken out, but Andrew might be able to answer
> that.
> >>> >
> >>> > On 08/07/2008, Aleksey Shipilev <aleksey.shipilev@gmail.com>
wrote:
> >>> >>
> >>> >> This method is the hell for performance.
> >>> >> It is not only accounts for 15% of CPU time, but instrumentation
> also
> >>> >> shows:
> >>> >> - average size of array is 4700 elements
> >>> >> - 99% times the search traverses entire array and finds nothing
> >>> >>
> >>> >> We should consider move to *Map here :)
> >>> >>
> >>> >> Thanks,
> >>> >> Aleksey.
> >>> >>
> >>> >> On Tue, Jul 8, 2008 at 1:30 PM, Sian January <
> >>> sianjanuary@googlemail.com>
> >>> >> wrote:
> >>> >> > The purpose of this code is to get the class constant pool
> ordering
> >>> >> right,
> >>> >> > so that if a signature string has already been transmitted
in the
> >>> CpUtf8
> >>> >> > band it has the correct global index.  However it might be
> possible
> >>> to do
> >>> >> > the search a different way if this method is taking a long
time.
>  E.g
> >>> if
> >>> >> I
> >>> >> > get rid of the code that differentiates between signatures
and
> other
> >>> >> Ascii
> >>> >> > values (i.e. replace all occurrences of
> >>> >> > ClassConstantPool.DOMAIN_SIGNATUREASCIIZ
> >>> >> > with ClassConstantPool.DOMAIN_NORMALASCIIZ) then my tests
seem to
> >>> pass,
> >>> >> > although Andrew you wrote that code so it might be that I'm
not
> >>> testing
> >>> >> all
> >>> >> > the cases where this is needed.  Also there might be worse
> >>> performance
> >>> >> > implications from making that change, but I can attach a patch
to
> the
> >>> >> JIRA
> >>> >> > if you would like to test it.
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >> > On 08/07/2008, Aleksey Shipilev (JIRA) <jira@apache.org>
wrote:
> >>> >> >>
> >>> >> >>
> >>> >> >>    [
> >>> >> >>
> >>> >>
> >>>
> https://issues.apache.org/jira/browse/HARMONY-5900?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12611488#action_12611488
> >>> >> ]
> >>> >> >>
> >>> >> >> Aleksey Shipilev commented on HARMONY-5900:
> >>> >> >> -------------------------------------------
> >>> >> >>
> >>> >> >> From my measurements for 20Mb .jar.pack.gz file on Sun
JDK
> 1.6.0_05
> >>> >> >> -server, CPBands.search accounts for 15% of CPU time.
> >>> >> >> Let's move the discussion to dev.
> >>> >> >>
> >>> >> >> > [classlib][pack200]
> >>> CpBands.parseCpSignature(Ljava/io/InputStream;) is
> >>> >> >> hot
> >>> >> >> >
> >>> >> >>
> >>> >>
> >>>
> --------------------------------------------------------------------------
> >>> >> >> >
> >>> >> >> >                 Key: HARMONY-5900
> >>> >> >> >                 URL:
> >>> >> https://issues.apache.org/jira/browse/HARMONY-5900
> >>> >> >> >             Project: Harmony
> >>> >> >> >          Issue Type: Wish
> >>> >> >> >          Components: Classlib
> >>> >> >> >    Affects Versions: 5.0M6
> >>> >> >> >         Environment: All Pack200 HEAD
> >>> >> >> >            Reporter: Andrew Cornwall
> >>> >> >> >
> >>> >> >> > The method
> >>> >> >>
> >>> >>
> >>>
> org/apache/harmony/unpack200/CpBands.parseCpSignature(Ljava/io/InputStream;)
> >>> >> >> appears to be very hot. I tried initially to optimize
it by
> caching
> >>> some
> >>> >> of
> >>> >> >> its arrays:
> >>> >> >> >     static void clearArrayCache() {
> >>> >> >> >       arrayCache = new SegmentConstantPoolArrayCache();
> >>> >> >> >     }
> >>> >> >> >
> >>> >> >> >     private static SegmentConstantPoolArrayCache
arrayCache =
> new
> >>> >> >> SegmentConstantPoolArrayCache();
> >>> >> >> >
> >>> >> >> >     private int search(String[] array, String string)
{
> >>> >> >> >       if(array.length > 30) {
> >>> >> >> >               List indexes =
> arrayCache.indexesForArrayKey(array,
> >>> >> >> string);
> >>> >> >> >               if (indexes.size() == 0) {
> >>> >> >> >                       return -1;
> >>> >> >> >               }
> >>> >> >> >               return ((Integer)indexes.get(0)).intValue();
> >>> >> >> >       } else {
> >>> >> >> >               for (int i = 0; i < array.length;
i++) {
> >>> >> >> >                       if(array[i].equals(string))
{
> >>> >> >> >                               return i;
> >>> >> >> >                       }
> >>> >> >> >               }
> >>> >> >> >               return -1;
> >>> >> >> >       }
> >>> >> >> >     }
> >>> >> >> > ... but that didn't appear to increase performance.
(Maybe all
> the
> >>> >> >> searches are done once?)
> >>> >> >> > Any ideas how to tune parseCpSignature to get it
faster?
> >>> >> >>
> >>> >> >> --
> >>> >> >> This message is automatically generated by JIRA.
> >>> >> >> -
> >>> >> >> You can reply to this email to add a comment to the issue
online.
> >>> >> >>
> >>> >> >>
> >>> >> >
> >>> >> >
> >>> >> > --
> >>> >> > Unless stated otherwise above:
> >>> >> > IBM United Kingdom Limited - Registered in England and Wales
with
> >>> number
> >>> >> > 741598.
> >>> >> > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
> >>> PO6
> >>> >> 3AU
> >>> >> >
> >>> >>
> >>> >
> >>> >
> >>> >
> >>> > --
> >>> > Unless stated otherwise above:
> >>> > IBM United Kingdom Limited - Registered in England and Wales with
> number
> >>> > 741598.
> >>> > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
> PO6
> >>> 3AU
> >>> >
> >>>
> >>
> >>
> >
>

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