harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aleksey Shipilev" <aleksey.shipi...@gmail.com>
Subject Re: [jira] Commented: (HARMONY-5900) [classlib][pack200] CpBands.parseCpSignature(Ljava/io/InputStream;) is hot
Date Wed, 09 Jul 2008 10:15:33 GMT
Andrew,

Sorry, there was a glitch in the patch: index is -1 always there.
The change is pretty trivial but I attached updated patch in HARMONY-5900.

Thanks,
Aleksey.

On Wed, Jul 9, 2008 at 3:21 AM, Andrew Cornwall
<andrew.pack200@gmail.com> wrote:
> 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
View raw message