|Summary:||Skip some useless information in the class when parsing|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
|Bug Depends on:||56953|
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 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 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.