Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#222 introduced a major performance regression #271

Open
csobrinho opened this issue Aug 20, 2019 · 6 comments
Open

#222 introduced a major performance regression #271

csobrinho opened this issue Aug 20, 2019 · 6 comments

Comments

@csobrinho
Copy link
Contributor

Hi Shigeru, #222 and 1719817 added a major regression to the CtClass loading. I profiled the code and now 75-80% of all the cpu time is wasted on "urlConnection.getInputStream()".

We saw a regression of at least 20x when the jar is big enough (120k classes, 400 MB jar).

  • with 3.23.1-GA, processing of all classes takes ~200s
  • with 3.25.0-GA, processing of all classes takes ~4200s

bytecode

@TimvdLippe
Copy link

Would it be possible to revert the commit for now? Seems like the performance regression is a lot more severe than the memory leak.

chibash added a commit that referenced this issue Sep 1, 2019
@chibash
Copy link
Member

chibash commented Sep 1, 2019

I've reverted the commit.

@csobrinho
Copy link
Contributor Author

Awesome @chibash. Any chance that commit could be cherrypicked into 3.25.0-GA as 3.25.1-GA or a new release be pushed? Thanks

@chibash
Copy link
Member

chibash commented Sep 5, 2019

It should be soon. by the end of September?

@csobrinho
Copy link
Contributor Author

Sounds good, thanks!

odl-github pushed a commit to opendaylight/odlparent that referenced this issue Oct 10, 2019
3.25 fixes:
- jboss-javassist/javassist#72
- jboss-javassist/javassist#241
- jboss-javassist/javassist#242
- jboss-javassist/javassist#246
- jboss-javassist/javassist#252
3.26 fixes:
- jboss-javassist/javassist#265
- jboss-javassist/javassist#270
- jboss-javassist/javassist#271
- jboss-javassist/javassist#275

Of these #270 is most important, as it fixes an issue we've seen
with powermock downstream.

Change-Id: Ib4d75d6411e71438436249a8eb9313ccf4411ca2
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
@csobrinho
Copy link
Contributor Author

Some more details. I tested 3.26.0-GA and it's faster (Thank you!) but still 10x slower than 3.22.0-GA.

When the JarClassPath.jarEntries is really big, the ArrayList.contains call inside the find will be O(n) and this will be very slow if done several thousand times.

In the profile picture we can see that almost all the time taken by ClassPool.openClassfile is consumed by List.contains.

If we only need to check if the class is there or not, then an HashSet will be much better and faster with O(1) checks. Also insertion order and index of the element are not used and necessary.

This made our compilation jump from 3m back to the original 20s in 3.22.0-GA.

Please be so kind and merge this as 3.26.1-GA.
Thank you!

List contains bottleneck

odl-github pushed a commit to opendaylight/odlparent that referenced this issue Oct 17, 2019
3.25 fixes:
- jboss-javassist/javassist#72
- jboss-javassist/javassist#241
- jboss-javassist/javassist#242
- jboss-javassist/javassist#246
- jboss-javassist/javassist#252
3.26 fixes:
- jboss-javassist/javassist#265
- jboss-javassist/javassist#270
- jboss-javassist/javassist#271
- jboss-javassist/javassist#275

Of these #270 is most important, as it fixes an issue we've seen
with powermock downstream.

Change-Id: Ib4d75d6411e71438436249a8eb9313ccf4411ca2
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 6a404f1)
odl-github pushed a commit to opendaylight/odlparent that referenced this issue Oct 17, 2019
3.25 fixes:
- jboss-javassist/javassist#72
- jboss-javassist/javassist#241
- jboss-javassist/javassist#242
- jboss-javassist/javassist#246
- jboss-javassist/javassist#252
3.26 fixes:
- jboss-javassist/javassist#265
- jboss-javassist/javassist#270
- jboss-javassist/javassist#271
- jboss-javassist/javassist#275

Of these #270 is most important, as it fixes an issue we've seen
with powermock downstream.

Change-Id: Ib4d75d6411e71438436249a8eb9313ccf4411ca2
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 6a404f1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants