JCI: don't load method instructions #3447

smowton · 2018-11-19T17:39:10Z

This speeds loading 40000 classes up by around 3x when the consumer isn't interested in the method bodies. (Use case: in the security product we found it useful to inspect class metadata using the same parser as will be used in jbmc, to ensure consistency between the two processes)

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 30aad53).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/91986750

thk123 · 2018-11-20T17:29:44Z

jbmc/unit/java_bytecode/java_bytecode_parser/parse_class_without_instructions.cpp

+      {
+        REQUIRE(loaded_class.parsed_class.methods.size() == 1);
+
+        const auto &method = *loaded_class.parsed_class.methods.begin();


💡 Add a check here that the loaded class at least has the member variable x
I would also extend to check:

accessibility of class

inner class loaded

accessibility of method

annotations on method

accessibility / annotations of fields

type hierarchy

a non constructor

thk123 · 2018-11-20T17:31:01Z

jbmc/src/java_bytecode/java_bytecode_parser.cpp

@@ -78,6 +80,7 @@ class java_bytecode_parsert:public parsert
  };

  std::vector<bytecodet> bytecodes;
+  bool skip_instructions;


⛏️

Suggested change

bool skip_instructions;

bool skip_instructions = false;

if someone adds a new constructor and forgets to initialise this field we won't be left in UB

smowton · 2018-11-22T11:39:19Z

@thk123 I've added a test for a non-constructor -- the other attributes ought to have nothing to do with the dont-load-instructions flag, so I won't include those in this test

thk123 · 2018-11-22T11:59:54Z

ought to have nothing to do with the dont-load-instructions flag

That's why I thought a test would be good, to ensure doesn't accidentally not load too much - but approved anyway

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 37539f2).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/92286213

smowton · 2018-11-22T14:58:31Z

Ah, got you -- added such checks

tautschnig · 2018-11-22T15:01:49Z

jbmc/unit/java_bytecode/java_bytecode_parser/parse_class_without_instructions.cpp

+  "java_bytecode_parse_class_without_instructions",
+  "[core][java_bytecode][java_bytecode_parser]")
+{
+  null_message_handlert null_out;


We have a global null_message_handler in the unit tests, no need for that one.

tautschnig · 2018-11-22T15:03:52Z

jbmc/src/java_bytecode/java_bytecode_parser.cpp

@@ -1215,7 +1218,7 @@ void java_bytecode_parsert::rmethod_attribute(methodt &method)

  irep_idt attribute_name=pool_entry(attribute_name_index).s;

-  if(attribute_name=="Code")
+  if(attribute_name=="Code" && !skip_instructions)


clang-format will require adding space around ==

tautschnig · 2018-11-22T15:05:50Z

jbmc/src/java_bytecode/java_bytecode_parser.cpp

  {
-    get_bytecodes();
+    if(!skip_instructions)
+      get_bytecodes();


I'm a bit surprised that we both need a class member and this branch here. But that may just be down to my limited understanding of the code here.

The method is badly named: this actually initialises a lookup table of possible bytecodes (only used within rbytecodes)

Maybe the if(!skip_instructions) should be moved into this method as if(skip_instructions) return; - you need to know what the method does in order to know whether you can skip it, and therefore to me it would seem better to place the condition with the implementation.

romainbrenguier · 2018-11-22T15:03:17Z

jbmc/src/java_bytecode/java_bytecode_parser.cpp

@@ -78,6 +80,7 @@ class java_bytecode_parsert:public parsert
  };

  std::vector<bytecodet> bytecodes;
+  bool skip_instructions = false;


romainbrenguier · 2018-11-22T15:05:16Z

jbmc/src/java_bytecode/java_bytecode_parser.h

@@ -16,10 +16,28 @@ Author: Daniel Kroening, kroening@kroening.com

 struct java_bytecode_parse_treet;

+/// Attempt to parse a Java class from the given file.
+/// \param file: file to load from
+/// \param msg: gets log messages


⛏️ "gets" doesn't seem right here, maybe "handles"?

romainbrenguier · 2018-11-22T15:06:22Z

jbmc/src/java_bytecode/java_bytecode_parser.h

+/// \param msg: gets log messages
+/// \param skip_instructions: if true, the loaded class's methods will all be
+///   empty. Saves time and memory for consumers that only want signature info.
+/// \return parse tree, or Empty on failure


⛏️ not sure "Empty" should be capitalized 🦅

romainbrenguier · 2018-11-22T15:07:11Z

jbmc/src/java_bytecode/java_bytecode_parser.h

+/// \param msg: gets log messages
+/// \param skip_instructions: if true, the loaded class's methods will all be
+///   empty. Saves time and memory for consumers that only want signature info.
+/// \return parse tree, or Empty on failure


romainbrenguier · 2018-11-22T15:08:30Z

jbmc/unit/java_bytecode/java_bytecode_parser/parse_class_without_instructions.cpp

+  REQUIRE(loaded_class.is_inner_class);
+  REQUIRE(loaded_class.outer_class == "Trivial");
+
+  const auto &fieldone = *loaded_class.fields.begin();


⛏️ field_one or field1

peterschrammel · 2018-11-22T15:52:49Z

jbmc/src/java_bytecode/java_bytecode_parser.h

-
+java_bytecode_parse(
+  const std::string &file,
+  class message_handlert &msg,


class keyword seems unnecessary (more occurrences)

This speeds loading 40000 classes up by around 3x.

allredj

🚫
This PR failed Diffblue compatibility checks (cbmc commit: e1388e1).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/92313175
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 8f7f8ae).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/92316672

smowton requested review from cesaro, cristina-david, NathanJPhillips, peterschrammel, pkesseli and thk123 as code owners November 19, 2018 17:39

smowton force-pushed the smowton/feature/java-parse-without-instructions branch from 055a9a8 to 30aad53 Compare November 20, 2018 09:48

allredj reviewed Nov 20, 2018

View reviewed changes

thk123 approved these changes Nov 20, 2018

View reviewed changes

thk123 reviewed Nov 20, 2018

View reviewed changes

smowton force-pushed the smowton/feature/java-parse-without-instructions branch from 30aad53 to 37539f2 Compare November 22, 2018 11:38

allredj reviewed Nov 22, 2018

View reviewed changes

smowton force-pushed the smowton/feature/java-parse-without-instructions branch from 37539f2 to 75d8ee3 Compare November 22, 2018 14:58

tautschnig reviewed Nov 22, 2018

View reviewed changes

romainbrenguier approved these changes Nov 22, 2018

View reviewed changes

smowton force-pushed the smowton/feature/java-parse-without-instructions branch from 75d8ee3 to e1388e1 Compare November 22, 2018 15:33

peterschrammel approved these changes Nov 22, 2018

View reviewed changes

JCI: don't load method instructions

8f7f8ae

This speeds loading 40000 classes up by around 3x.

smowton force-pushed the smowton/feature/java-parse-without-instructions branch from e1388e1 to 8f7f8ae Compare November 22, 2018 16:18

allredj reviewed Nov 22, 2018

View reviewed changes

smowton merged commit d7645fd into diffblue:develop Nov 23, 2018

JCI: don't load method instructions #3447

JCI: don't load method instructions #3447

Uh oh!

Conversation

smowton commented Nov 19, 2018

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Nov 22, 2018

Uh oh!

thk123 commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

smowton commented Nov 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thk123 commented Nov 22, 2018 •

edited

Loading