Hi Gary (and BCEL maintainers),

Thank you for the comment. It has been addressed. Would you check the pull
request?
https://github.com/apache/commons-bcel/pull/26/files

Regards,
Tomo

On Wed, May 8, 2019 at 17:46 Tomo Suzuki <suzt...@google.com> wrote:

> Hi Gary,
> Created a draft PR to receive feedback.
> https://github.com/apache/commons-bcel/pull/26/files . What do you
> think?
>
> Regards,
> Tomo
>
> From: Gary Gregory <garydgreg...@gmail.com>
> Date: Wed, May 8, 2019 at 9:40 AM
> To: Commons Developers List
>
> > Great. I look forward to seeing what you'll come up with :-)
> >
> > On Tue, May 7, 2019 at 4:27 PM Tomo Suzuki <suzt...@google.com.invalid>
> > wrote:
> >
> > > I found the discussion on getInstance method had incurred performance
> > > degradation https://issues.apache.org/jira/browse/BCEL-186 .
> > >
> > > From the JIRA:
> > > > This feature could return as a pluggable cache if someone wants to
> > > provide a patch
> > >
> > > I'll think about the approach.
> > >
> > > Regards,
> > > Tomo
> > >
> > >
> > > On Thu, May 2, 2019 at 1:22 PM Gary Gregory <garydgreg...@gmail.com>
> > > wrote:
> > >
> > > > On Thu, May 2, 2019 at 10:59 AM Tomo Suzuki
> <suzt...@google.com.invalid>
> > > > wrote:
> > > >
> > > > > Gary,
> > > > > I didn't find ConstantUtf8.getCachedInstance
> > > > > <
> > > >
> > >
> https://github.com/apache/commons-bcel/blob/master/src/main/java/org/apache/bcel/classfile/ConstantUtf8.java#L94
> > > > >
> > > > > is used in BCEL (latest 6.3.1). I tried my custom ConstantUtf8
> > > > modification
> > > > > to leverage getCachedInstance method and it worked well:
> > > > >
> > > > > Before
> > > > >
> > > > > Without getCachedInstance, my tool created 2,6 million ConstantUtf8
> > > > > instances (before failing OutOfMemoryError: GC overhead limit
> exceeded)
> > > > to
> > > > > check ~200 jar files
> > > > > [image: 2644k_constantutf8_without_getCachedInstance.png]
> > > > >
> > > > >
> > > > > After
> > > > >
> > > > > With getCachedInstance, my tool created just 0.6 million
> ConstantUtf8
> > > > > instances. It didn't throw the OutOfMemoryError.
> > > > > [image: 621k_constantutf8_with_getCachedInstance.png]
> > > > >
> > > > > My modification was to use getCachedInstance for
> > > > > ConstantUtf8.getInstance(DataInput)
> > > > > <
> > > >
> > >
> https://github.com/apache/commons-bcel/blob/master/src/main/java/org/apache/bcel/classfile/ConstantUtf8.java#L123
> > > > >
> > > > > .
> > > > >
> > > > >
> > > > > Do you know why ConstantUtf8.getCachedInstance is unused?
> > > > >
> > > >
> > > > I do not.
> > > >
> > > > Gary
> > > >
> > > >
> > > > >
> > > > > Regards,
> > > > > Tomo
> > > > >
> > > > >
> > > > >
> > > > > On Wed, May 1, 2019 at 7:02 PM Gary Gregory <
> garydgreg...@gmail.com>
> > > > > wrote:
> > > > >
> > > > >> On Wed, May 1, 2019 at 6:37 PM Tomo Suzuki
> <suzt...@google.com.invalid
> > > >
> > > > >> wrote:
> > > > >>
> > > > >> > Gary,
> > > > >> > That’s right. I missed it. I think I just need to find a way to
> > > > leverage
> > > > >> > the getCachedInstance method. Thank you for quick response.
> > > > >> >
> > > > >>
> > > > >> YW. Please let us know your outcome.
> > > > >>
> > > > >> Gary
> > > > >>
> > > > >>
> > > > >> >
> > > > >> > On Wed, May 1, 2019 at 18:00 Gary Gregory <
> garydgreg...@gmail.com>
> > > > >> wrote:
> > > > >> >
> > > > >> > > Why not use getCachedInstance() ?
> > > > >> > >
> > > > >> > > Gary
> > > > >> > >
> > > > >> > > On Wed, May 1, 2019 at 5:20 PM Tomo Suzuki
> > > > <suzt...@google.com.invalid
> > > > >> >
> > > > >> > > wrote:
> > > > >> > >
> > > > >> > > > Hi BCEL developers,
> > > > >> > > >
> > > > >> > > > We use BCEL library to inspect Java class. Thank you for the
> > > great
> > > > >> > > library.
> > > > >> > > >
> > > > >> > > > When our tool checks classes in ~200 jar files, it creates
> more
> > > > >> than 2
> > > > >> > > > million BCEL ConstantUtf8 instances. I suspect many of them
> > > share
> > > > >> the
> > > > >> > > same
> > > > >> > > > values such as "java.lang.String".
> > > > >> > > >
> > > > >> > > > [image: many_constant_utf8.png]
> > > > >> > > >
> > > > >> > > > Given this observation, I got an idea to to reuse
> ConstantUtf8
> > > > with
> > > > >> > same
> > > > >> > > > value to save memory footprint. Because ConstantUtf8 is
> > > constant,
> > > > >> > sharing
> > > > >> > > > the instances across classes should not cause a problem.
> Note
> > > that
> > > > >> BCEL
> > > > >> > > is
> > > > >> > > > not designed thread-safe (doc
> > > > >> > > > <https://commons.apache.org/proper/commons-bcel/faq.html>).
> > > > >> > > >
> > > > >> > > > If this is a good idea, I'm thinking to make a pull request
> > > around
> > > > >> > BCEL's
> > > > >> > > > ConstantUtf8.getInstance method:
> > > > >> > > >
> > > > >> > > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > >
> > >
> https://github.com/apache/commons-bcel/blob/master/src/main/java/org/apache/bcel/classfile/ConstantUtf8.java#L116
> > > > >> > > >
> > > > >> > > > Does anybody have any thought?
> > > > >> > > >
> > > > >> > > > --
> > > > >> > > > Regards,
> > > > >> > > > Tomo
> > > > >> > > >
> > > > >> > >
> > > > >> > --
> > > > >> > Regards,
> > > > >> > Tomo
> > > > >> >
> > > > >>
> > > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Tomo
> > > > >
> > > >
> > >
> > >
> > > --
> > > Regards,
> > > Tomo
> > >
>
>
>
> --
> Regards,
> Tomo
>
-- 
Regards,
Tomo

Reply via email to