Speedup #30

altaite · 2017-03-06T20:10:01Z

Several changes in decoder, resulting in about 4x speedup of BioJava MMTF parsing. Largest change (but not most significant in terms of time) is replacing Jackson with Jmol version of MessagePack parsing (sources were extracted, no dependency). A test for that was created, comparing objects created by both methods.

hopefully every other OS.

available by a switch in MessagePackSerialization.

of BioJava Structure creation.

Passed MessagePackTest.

parser adopted from Jmol are identical.

coveralls · 2017-03-06T20:12:16Z

Coverage decreased (-17.9%) to 74.114% when pulling 1570040 on altaite:speedup into 7d9a709 on rcsb:master.

josemduarte · 2017-03-07T15:55:14Z

Thanks. I'm trying to review it, give me some time. As an advice for next time, please don't do code reformatting at the same time that you do important changes. The diffs as they are shown are not very helpful.

Having said that, there's a really nice trick to ignore whitespaces in the diff that will make them more readable: add a ?w=1 at the end of the URL (see https://github.com/blog/967-github-secrets). For this PR's diff it would be: https://github.com/rcsb/mmtf-java/pull/30/files?w=1

josemduarte

It looks to me like the file pdb_entry_type.txt shouldn't be here, it's quite a large file. Is it needed?

josemduarte · 2017-03-07T16:02:24Z

mmtf-codec/src/test/java/org/rcsb/mmtf/encoder/TestEncoderUtils.java

+			return new GenericDecoder(ReaderUtils.getDataFromFile(inFile));
+		} catch (IOException e) {
+			e.printStackTrace();
+			throw new RuntimeException();


This is not your change, but it'd be nice to fix: don't catch the exception, just declare a "throws IOException" and error handling will be much clearer.

Text files are in gz now.

josemduarte · 2017-03-08T05:28:56Z

mmtf-api/pom.xml

+	<parent>
+		<groupId>org.rcsb</groupId>
+		<artifactId>mmtf</artifactId>
+		<version>1.0.5-SNAPSHOT</version>


No need to change the version here, SNAPSHOT versions are for development. Versions in pom should only change when a release happens. Same applies to other pom files.

josemduarte · 2017-03-08T05:38:05Z

mmtf-codec/src/test/java/org/rcsb/mmtf/serialization/mp/LineFile.java

+				lines.add(line);
+			}
+			return lines;
+		} catch (IOException e) {


Better to declare the exception as thrown in the method signature. Error handling will be easier like that

josemduarte · 2017-03-08T05:39:26Z

mmtf-codec/src/test/java/org/rcsb/mmtf/serialization/mp/MessagePackTest.java

+			}
+			return codes;
+		} catch (Exception ex) {
+			throw new RuntimeException(ex);


Just declare the exception as throws in signature. It will have the same effect and will be clearer

josemduarte · 2017-03-08T05:41:25Z

mmtf-codec/src/test/java/org/rcsb/mmtf/serialization/mp/MessagePackTest.java

+			MessagePackSerialization.setJackson(true);
+			StructureDataInterface sdiJackson = parse(bytes);
+
+			ReflectionAssert.assertReflectionEquals(sdiJackson, sdiJmol);


Is there a reason to use this assert method instead of the standard assert from the junit library?

This assert performs deep comparison, while standard assert just checks boolean.

josemduarte · 2017-03-08T05:42:50Z

mmtf-codec/src/test/java/org/rcsb/mmtf/serialization/mp/MessagePackTest.java

+ * creation gives identical data.
+ *
+ */
+public class MessagePackTest extends TestCase {


Better use junit4. It doesn't require extending TestCase, just adding the @test annotation to the method you want to make into a test

josemduarte · 2017-03-08T05:46:01Z

mmtf-serialization/pom.xml

+            <groupId>org.unitils</groupId>
+            <artifactId>unitils-core</artifactId>
+            <version>3.4.2</version>
+            <scope>test</scope>


Is this really needed? Can't we just use junit4?

Yes, for the deep comparison of objects.

josemduarte · 2017-03-08T05:47:53Z

mmtf-serialization/src/main/java/org/rcsb/mmtf/serialization/MessagePackSerialization.java

+			mmtfBean = objectMapper.readValue(inputStream, MmtfStructure.class);
+		} catch (IOException e) {
+			e.printStackTrace();
+		}


Exception should be declared in signature and not caught

josemduarte · 2017-03-08T05:48:24Z

mmtf-serialization/src/main/java/org/rcsb/mmtf/serialization/MessagePackSerialization.java

+			return s;
+		} catch (Exception e) {
+			throw new RuntimeException(e);
+		}


Declare IOException in signature

josemduarte · 2017-03-08T05:48:43Z

mmtf-serialization/src/main/java/org/rcsb/mmtf/serialization/MessagePackSerialization.java

+		try {
+			objectMapper.writeValue(outputStream, mmtfStructure);
+		} catch (IOException e) {
+			e.printStackTrace();


Don't catch the exception, declare it in signature

josemduarte · 2017-03-08T06:09:47Z

One concern is the 17.9% drop in test coverage, I guess because of the new message pack decoding code from jmol that has no tests. Do you have some metrics on the performance difference of the new decoder versus the msgpack library? It's a pity to lose a well-tested library in favor of an ad-hoc decoder.

Anyway thanks a lot for the patch! looks good in general. If you could address the comments that'd be great.

coveralls · 2017-03-09T21:46:45Z

Coverage decreased (-17.9%) to 74.085% when pulling 3822c89 on altaite:speedup into 7d9a709 on rcsb:master.

RuntimeExceptions by IOException and try with resources.

josemduarte · 2017-03-17T21:17:50Z

mmtf-codec/src/test/java/org/rcsb/mmtf/serialization/mp/ZippedTextFile.java

+		}
+	}
+
+}


You need gzipping rather than zipping here.

This is how you do it with gzipping:

public static void gzipFile(File inFile, File outFile) throws IOException { GZIPOutputStream zos = new GZIPOutputStream(new FileOutputStream(outFile)); FileInputStream is = new FileInputStream(inFile); int b; while ( (b=is.read())!=-1) { zos.write(b); } zos.close(); is.close(); }

And ungzip is:

public static void gunzipFile(File inFile, File outFile) throws IOException { GZIPInputStream zis = new GZIPInputStream(new FileInputStream(inFile)); FileOutputStream os = new FileOutputStream(outFile); int b; while ( (b=zis.read())!=-1) { os.write(b); } zis.close(); os.close(); }

I would say that my code is probably faster (buffer). And more importantly, it is easier to prepare the resource, because I do not need to do it with Java. I just do it by file manager or command line, which is far more convenient. Also I would leave it because it does not matter much I think.

The issue here is zip vs gzip, not so much about efficiency. Zip is an archive format (to put a bunch of files together), while gzip is purely a compression format.

You can also do gzip easily from command line with tools like gzip and gunzip, so that's also not a concern.

previous was failing on Windows again. Going to use URL elsewhere too in the next commit.

whenever possible (simpler and more reliable), File left only when necessary for API tests.

altaite · 2017-03-23T00:08:48Z

Concerning test coverage (I do not see way how to comment directly):
There is a test which allows to compare the new code creates exactly the same objects as Jackson, on the whole PDB: MessagePackTest. But it must be in different subproject mmtf-codec because of dependencies. Is there some easy way how to connect this to the code, so that coverage would be better? It is integration test, but it test equality of Jackson and new code completely, if you run it on whole database.

josemduarte · 2017-03-23T01:51:41Z

Thanks for all the changes! the only thing missing is putting back the version to 1.0.4-SNAPSHOT. After that I think we should merge. By the way there's a merge conflict now, could you take care of that too? (just merge into this branch the latest master). The conflict should be minimal, the only file that diverged since you branched is mmtf-codec/src/main/java/org/rcsb/mmtf/decoder/ReaderUtils.java

josemduarte · 2017-03-23T01:53:07Z

...ialization/src/main/java/org/rcsb/mmtf/serialization/quickmessagepackdeserialization/SB.java

+ * 
+ */
+
+public class SB {


Just found this one: could we get rid of it? This looks like a replacement for StringBuilder needed in jmol so that the java to js transpiler is happy. We don't need it here, let's replace it by StringBuilder

josemduarte · 2017-03-23T01:59:12Z

...on/src/main/java/org/rcsb/mmtf/serialization/quickmessagepackdeserialization/ObjectTree.java

+ *
+ * @author Antonin Pavelka
+ */
+public class ObjectTree {


Could we have some documentation for this class? or at least change the method names to something more usable, one letter method names without javadocs are going to be difficult to understand....

josemduarte · 2017-03-23T01:59:33Z

...on/src/main/java/org/rcsb/mmtf/serialization/quickmessagepackdeserialization/ObjectTree.java

+		}
+	}
+
+	public Hashtable l(String s) {


This method doesn't seem to be used. Do we need it?

josemduarte · 2017-03-23T02:10:28Z

mmtf-serialization/src/main/java/org/rcsb/mmtf/dataholders/MmtfStructureFactory.java

+ *
+ * @author Antonin Pavelka
+ */
+public class MmtfStructureFactory {


Could you add some javadocs to explain what this class does? The individual methods are more self-explanatory

… to one class.

…after 12/01/2016). Increased -Xmx for testing from 1500 to 6000 MB because sometimes GC timeout occured in MessagePackTest. Improved comments on the MessagePack parser.

was corrected to reasonable number 10.

coveralls · 2017-03-27T23:10:37Z

Coverage decreased (-11.6%) to 80.377% when pulling cd39c88 on altaite:speedup into 8aa07a8 on rcsb:master.

coveralls · 2017-03-28T01:36:14Z

Coverage decreased (-11.5%) to 80.479% when pulling 457bf2c on altaite:speedup into 8aa07a8 on rcsb:master.

coveralls · 2017-03-28T01:49:19Z

Coverage decreased (-11.4%) to 80.58% when pulling 07425c1 on altaite:speedup into 8aa07a8 on rcsb:master.

josemduarte

This looks really good now, thank you so much for all the changes. Just one final detail: I'd propose method names for ObjectTree like getString, getInteger, getByteArray etc. As I said, single letter names make things difficult to understand. Let's merge after you refactor those, it should only take a couple of minutes.

coveralls · 2017-03-28T03:57:25Z

Coverage decreased (-11.4%) to 80.58% when pulling 72e468e on altaite:speedup into 8aa07a8 on rcsb:master.

altaite added 7 commits February 28, 2017 13:41

Modified access to resource files so that it works on Windows and

d23bbd9

hopefully every other OS.

Replaced Jackson by MessagePack parsing code from Jmol. Jackson remains

9c4052b

available by a switch in MessagePackSerialization.

Moving code from getNumBonds() to constructor results in 4-fold speedup

36d075f

of BioJava Structure creation.

FLOAT64 is now parsed as double, which corresponds to what Jackson did.

5ce9fbc

Passed MessagePackTest.

Added a test comparing if results of Jackson and the new MessagePack

7c10d24

parser adopted from Jmol are identical.

Moved MessagePack test into mmtf-codec.

c99723c

Version set to 1.05.

1570040

josemduarte requested changes Mar 8, 2017

View reviewed changes

josemduarte reviewed Mar 8, 2017

View reviewed changes

josemduarte requested changes Mar 8, 2017

View reviewed changes

Renaming of mp package to quickmessagepackdeserialization.

3822c89

Zipped resource pdb_entry_type.txt. Replaced few other

86c7a03

RuntimeExceptions by IOException and try with resources.

josemduarte reviewed Mar 17, 2017

View reviewed changes

altaite added 3 commits March 17, 2017 22:26

Replaced zip with gz. Also corrected the loading of file using URL,

06455cb

previous was failing on Windows again. Going to use URL elsewhere too in the next commit.

Nicer code for reading text files as resources in gz. URL are used

5fc6b9a

whenever possible (simpler and more reliable), File left only when necessary for API tests.

Minor formatting changes and removal of dead code in comments.

5f8057f

josemduarte reviewed Mar 23, 2017

View reviewed changes

altaite added 4 commits March 23, 2017 16:17

Message Pack parsing code from Bob Hanson's Jmol simplified and moved…

acf8e1c

… to one class.

PDB codes for testing now does not contain missing records (no codes …

032d454

…after 12/01/2016). Increased -Xmx for testing from 1500 to 6000 MB because sometimes GC timeout occured in MessagePackTest. Improved comments on the MessagePack parser.

Correction of versions to 1.0.4-SNAPSHOT.

2999e37

Better API for Lines utility.

4f69622

altaite added 3 commits March 27, 2017 15:49

Merge branch 'master' into speedup

ef9034f

Better API for Lines utility. Also number of tested structures

2d388ef

was corrected to reasonable number 10.

Merge origin/speedup into speedup

cd39c88

Replaced Exception for ParseException in MessagePack decoding.

457bf2c

More appropriate name for test.

07425c1

josemduarte requested changes Mar 28, 2017

View reviewed changes

More descriptive method names in ObjectTree.

72e468e

josemduarte approved these changes Mar 28, 2017

View reviewed changes

josemduarte merged commit 2978574 into rcsb:master Mar 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speedup #30

Speedup #30

altaite commented Mar 6, 2017

coveralls commented Mar 6, 2017

josemduarte commented Mar 7, 2017 •

edited

josemduarte left a comment

josemduarte Mar 7, 2017

altaite Mar 22, 2017

josemduarte Mar 8, 2017

josemduarte Mar 8, 2017

josemduarte Mar 8, 2017

josemduarte Mar 8, 2017

altaite Mar 22, 2017

josemduarte Mar 8, 2017

josemduarte Mar 8, 2017

altaite Mar 22, 2017

josemduarte Mar 8, 2017

josemduarte Mar 8, 2017

josemduarte Mar 8, 2017

josemduarte commented Mar 8, 2017

coveralls commented Mar 9, 2017

josemduarte Mar 17, 2017

altaite Mar 17, 2017

josemduarte Mar 17, 2017

altaite commented Mar 23, 2017

josemduarte commented Mar 23, 2017

josemduarte Mar 23, 2017

josemduarte Mar 23, 2017

josemduarte Mar 23, 2017

josemduarte Mar 23, 2017

coveralls commented Mar 27, 2017

coveralls commented Mar 28, 2017

coveralls commented Mar 28, 2017

josemduarte left a comment

coveralls commented Mar 28, 2017

+              		}
+              	}
+              }

Speedup #30

Speedup #30

Conversation

altaite commented Mar 6, 2017

coveralls commented Mar 6, 2017

josemduarte commented Mar 7, 2017 • edited

josemduarte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemduarte commented Mar 8, 2017

coveralls commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altaite commented Mar 23, 2017

josemduarte commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 27, 2017

coveralls commented Mar 28, 2017

coveralls commented Mar 28, 2017

josemduarte left a comment

Choose a reason for hiding this comment

coveralls commented Mar 28, 2017

josemduarte commented Mar 7, 2017 •

edited