Bug 56955 - Skip some useless information in the class when parsing
Summary: Skip some useless information in the class when parsing
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: trunk
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on: 56953
Blocks:
  Show dependency tree
 
Reported: 2014-09-11 06:59 UTC by hzhang9
Modified: 2014-09-12 13:26 UTC (History)
0 users



Attachments
patch (117.10 KB, patch)
2014-09-11 06:59 UTC, hzhang9
Details | Diff
test case (4.58 KB, text/plain)
2014-09-11 07:01 UTC, hzhang9
Details
patch (130.82 KB, patch)
2014-09-12 05:20 UTC, hzhang9
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description hzhang9 2014-09-11 06:59:59 UTC
Created attachment 31996 [details]
patch

“org.apache.tomcat.util.bcel.classfile.ClassParser” parse ".class" file to an Object.
Actually, many information in the ".class" is useless. Including: version, fields, methods.

Skip these information can speed up the startup of tomcat. Add a switch named "isSkip", and modify the parse as following.

 /****************** Read headers ********************************/
      // Check magic tag of class file
      readID();
      // Get compiler version
      if(!isSkip) readVersion();
      else skipVersion();
      /****************** Read constant pool and related **************/
      // Read constant pool entries
      readConstantPool();
      // Get class information
      readClassInfo();
      // Get interface information, i.e., implemented interfaces
      readInterfaces();
      /****************** Read class fields and methods ***************/
      // Read class fields, i.e., the variables of the class
      if(!isSkip) readFields();
      else skipFields();
      // Read class methods, i.e., the functions in the class
      if(!isSkip) readMethods();
      else skipMethods();
      // Read class attributes
      readAttributes();
Comment 1 hzhang9 2014-09-11 07:01:37 UTC
Created attachment 31997 [details]
test case
Comment 2 hzhang9 2014-09-11 07:08:02 UTC
Benefit (got by test case in the attachment):

=====lots of jar files=====
Normal: 551
Skip: 453
=====few jar files=====
Normal: 88
Skip: 65

notice: 
1.Skip version calls before original version in the test case, so the performance should be better.
2.It works more efficient in large projects.
Comment 3 Mark Thomas 2014-09-11 09:04:41 UTC
Several comments.

A number of these changes have already been made in 8.0.x and can be back-ported. svn revisions to back-port are therefore preferred to new patches.

There is no need to make skipping optional. The information is either required (in which case we keep it) or not (in which case we dump it).

Unless a bug is specific to a major release, patches should always be against tomcat/trunk in svn and will be back-ported to the version the bug was opened against.

This bug report prompted me to review (again) what information we keep in BCEL and I have removed a largish chunk that isn't covered by this patch.

I'll take another look at the class parser next.
Comment 4 Mark Thomas 2014-09-11 10:33:06 UTC
Some further comments:

The patch should address a single concern. Most of the provided patch has nothing to do with this bug report.

There are a lot of changes that appear to undo previous clean-up.

I recommend testing with tomcat/trunk (currently the 8.0.x development branch). A lot more changes have been made to that branch that haven't yet been backported to 7.0.x.
Comment 5 hzhang9 2014-09-12 05:20:32 UTC
Created attachment 32009 [details]
patch
Comment 6 Mark Thomas 2014-09-12 07:52:59 UTC
If you continue to ignore the comments you are given then this issue is going to get closed as WONTFIX.

Your first patch was 117k and full of irrelevant changes. The second attempt is worse at 130k.

Starting at the beginning of the patch:
1. The first chunk changes restores an svn keyword the Tomcat team previously removed. This change has nothing to do with this issue and should not be in this patch.

2. The second chunk reverts a fix to a constant name the Tomcat team previously fixed and removes some code necessary for Java 8 support. This change has nothing to do with this issue and should not be in this patch. Further this change breaks Java 8 support.

3. The third chunk makes further changes that have nothing to do with this issue and further breaks Java 8 support.

And so on.

To repeat:
- Patches should be against http://svn.apache.org/viewvc/tomcat/trunk/
- If you'd like to propose changes to back port form there to tc7.0.x/trunk then provide some revision numbers.
Comment 7 hzhang9 2014-09-12 08:39:15 UTC
Sorry for any inconvenience. I am now reviewing my code.
Comment 8 Mark Thomas 2014-09-12 13:26:20 UTC
Numerous BCEL changes have been back-ported from trunk for the next 7.0.x release including ones that cover this report.

The new code will be in 7.0.56 onwards.