Faster xml parsing #238
Faster xml parsing #238
Conversation
* Reflection was very slow, and threw many exceptions as the code
"guessed" which methods might exist on a target class.
* Make all the types of objects built by the model builder explicit.
|
Seems like I keep failing these external tests every time I submit a pull request. I also find it hard to do an mvn clean install on droid now - it keeps failing to download NVD CVEs. My internet connection is fine - I'm not behind a proxy or something. [ERROR] Failed to execute goal org.owasp:dependency-check-maven:3.1.2:check (default) on project droid-parent: |
|
@nishihatapalmer Can you correct the checkstyle errors in |
|
Will do. I've had to disable the owasp plugin locally - it always fails to download its CVEs for some reason. Do I need to resubmit a new pull request for this, or is there a way to append any fixes to this PR? |
|
@nishihatapalmer I would recommend using I am looking into updating the OWASP plugin... |
|
Thanks. I've updated and fixed all the checkstyle errors, except one I can't seem to get around. The ClassDataAbstractionCoupling checkstyle rule limits the number of imported classes to 9, but we need more than this to instantiate all the different types of object in the XML. The original class didn't have this problem because it used reflection to instantiate them, so it wasn't obvious what was being imported. How can I disable this rule for this class? I've tried putting CHECKSTYLE:OFF around the imports, and around the class definition, but this doesn't help. |
|
I've tried disabling Checkstyle for the entire class, but then I get a license header violation (I guess because that prevents the license check from operating). The whole purpose of this XML parser is to instantiate a whole lot of other objects, so we're going to violate this checkstyle rule whatever we do. Help! |
|
OK - something weird is going on here with the build system. I tried adding a suppression for the checkstyle rule in suppressions.xml: When I do this, I don't get the ClassDataAbstractionCoupling error anymore... but I get a license check error instead: Failed to execute goal com.mycila:license-maven-plugin:3.0:check (check-headers) on project droid: Some files do not have the expected license header |
|
Giving up for the night. I may push the changes I have which fix the obvious checkstyle errors. This one looks like some weird kind of interaction with the different build plugins. |
|
OK. I've pushed the commit for the checkstyle errors I can fix. I don't know what to do with the ClassDataAbstractionCoupling / licence check problems. |
|
A bit more detail - adding the suppression for ClassDataAbstractionCoupling to suppressions.xml does allow SAXModelBuilder, and the whole core project to successfully compile. The parser class has to reference all the objects it instantiates, so it's reasonable to suppress this checkstyle rule for this class. The license header violation is in the droid project, not core. This seems to be something else, possibly something in my local repo I've changed. I'll push the suppression.xml change to this PR. |
* The SAXModelBuilder instantiates all the objects needed by signature files.
This violates the checkstyle rule that objects shouldn't have more than 9 dependencies on other classes,
so we suppress this rule for that class. It has to be able to instantiate all the objects.
* Missing //CHECKSTYLE:ON after turning off for method.
|
Solved license header failure - it was a local temporary text file I'd created in the project root when debugging the checkstyle errors! So the current PR should now be good. |
Almost doubles the speed of parsing signature files - creating a new profile is now much faster.
The original SAXModelBuilder class used reflection to guess what classes and methods existed.
The majority of time in parsing was spent calling Class.forName().newInstance() to create new instances of the objects parsed in DROID XML files. The rest of the time was spent guessing which methods existed on those classes, catching exceptions if they didn't and guessing a different method, up to 4 times per method. Not only is this ugly, it is also very slow.
I have made instantiating the objects explicit, along with explicitly calling the correct methods on each target class to set the properties. If a property doesn't exist, a warning is logged.
The only real downside of this new approach is that if you change the names of the classes, or add new core XML elements to the signature file formats, then you will have to update the SAXModelBuilder. The signature file formats have been very stable however - I'm not sure they have been updated in the last decade. I believe this approach is cleaner, more explicit and it's not hard to add new classes or methods if required.