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 23:21:21 GMT
Errm, I might take that back. I'm seeing a failure in
ArchiveTest.testWithAnnotations:

java.lang.NullPointerException
    at
org.apache.harmony.unpack200.CpBands.cpNameAndTypeValue(CpBands.java:776)
    at
org.apache.harmony.unpack200.MetadataBandGroup.getNextValue(MetadataBandGroup.java:225)
    at
org.apache.harmony.unpack200.MetadataBandGroup.getAnnotation(MetadataBandGroup.java:199)
    at
org.apache.harmony.unpack200.MetadataBandGroup.getAttribute(MetadataBandGroup.java:167)
    at
org.apache.harmony.unpack200.MetadataBandGroup.getAttributes(MetadataBandGroup.java:140)
    at
org.apache.harmony.unpack200.ClassBands.parseClassMetadataBands(ClassBands.java:1204)
    at
org.apache.harmony.unpack200.ClassBands.parseClassAttrBands(ClassBands.java:518)
    at org.apache.harmony.unpack200.ClassBands.unpack(ClassBands.java:152)
    at org.apache.harmony.unpack200.Segment.parseSegment(Segment.java:385)
    at org.apache.harmony.unpack200.Segment.unpack(Segment.java:405)
    at org.apache.harmony.unpack200.Archive.unpack(Archive.java:159)
    at
org.apache.harmony.unpack200.tests.ArchiveTest.testWithAnnotations(ArchiveTest.java:147)

Looks as if something's missing: the compiler is flagging:
            cpNameAndType = new CPNameAndType(name, descriptorU, domain,
index.intValue() + descrOffset);

with: "The variable index can only be null at this location".

    Andrew Jr.

On Tue, Jul 8, 2008 at 10:03 AM, Andrew Cornwall <andrew.pack200@gmail.com>
wrote:

> 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