Compile(Reader) and Compile(String)

[Squareys]

Hello @dscho, @ctrueden, hello all!

I am currently working on a scripting node for KNIME and very happy to be able to use scijava and scripting-java.
Current goal is to execute one Command Plugin for every row in an input table.
This requires the Plugins Java code (as String) to be compiled once and then be reused for every row.

Until now there was no way to do this (except use eval(String) for the first row and the load the class manually to run the command on the following rows). Since there is an compile(File, Writer) already, I suggest adding compile(String) and compile(Reader) aswell.
I replaced the code which does the compilation in eval(Reader) with a call to the new compile(Reader) method.

Open for discussion:

• I set the return value of compile to Class<?>. Other possibilities could be the name of the main class or void, analog to compile(File, Writer)
• Shouldn't compile(File, Writer) use getContext().getErrorWriter() ?
• I moved the class loader modification code to compile aswell ("needed for sezpoz"?), please pay special attention to whether that makes sense when reviewing.

What are your thoughts?

Greetings,
Jonathan

[dscho]

I set the return value of compile to Class<?>. Other possibilities could be the name of the main class or void, analog to compile(File, Writer)

I think it makes most sense to do it that way.

Shouldn't compile(File, Writer) use getContext().getErrorWriter() ?

Yes!

I moved the class loader modification code to compile aswell ("needed for sezpoz"?), please pay special attention to whether that makes sense when reviewing.

"Needed for annotation processing" now... we don't use Sezpoz anymore. And actually, as far as scripting-java is concerned, we do not need to pay attention to annotation processing anyway, because the PluginService is already initialized at that point and we have to "discover" the plugin (if it is a plugin) in a different way anyhow.

[dscho]

Oh, and a very fundamental comment: the way to prevent frequent recompiling would be to remember intermediate compilations of the identical source code.

To this end, it might make sense to turn the logic around, from passing a StringReader to the compile() method that takes a Reader to passing a String to the other method. That way, we could first checksum the code and pick a name for the temporary directory based on that checksum. It could then detect whether the code had been compiled already and just reuse it. The responsible javaService could even have a weak hash map of class loaders associated with temporary directories...

[Squareys]

Thank you for reviewing! I'll fix the issues you mentioned tomorrow, I don't know if I'll be able to do the recompilation checking though (timewise), I'll will try, though.

[Squareys]

@dscho I did not do the recompilation checking. I might do in the future, but in a separate pull request.
I went over the compile(File,Writer), which also returns the compiled Class now, to keep the compile methods a little more consistent.

[ctrueden]

@Squareys Still planning to work on this further? Do you understand what @dscho is asking you to do?

[Squareys]

Not in too near future, but yes. This is related to a task I am doing for Knip/@dietzc , which is currently on hold for another, so I cannot promise that this will be done too soon.

Is there any specific reason you ask?

[ctrueden]

@Squareys I was just going back through all the pending PRs, trying to resolve as many as possible, and categorizing those that remain. In this case, I just wanted to make sure we were all on the same page that the ball is currently in your court. Let us know if you need any help with it!

[dietzc]

@Squareys @ctrueden maybe we get some minutes on saturday to discuss whats missing that we can merge this PR.

[Squareys]

@dietzc what's missing is discussed in previous comments:
the way to prevent frequent recompiling would be to remember intermediate compilations of the identical source code (#3 (comment))

[dscho]

@Squareys an example for checksumming a string:

private final static Charset utf8 = Charset.fromName("UTF-8");
private final static MessageDigest sha1 = MessageDigest.getInstance("SHA-1");
private static String hash(final String code) {
    final InputStream input = new ByteArrayInputStream(code.getBytes(utf8));
    synchronized (sha1) {
        sha1.reset();
        final InputStream digestStream = new DigestInputStream(input, sha1);
        while (digestStream.read(buffer) >= 0); /* do nothing */
        digestStream.close();
        final byte[] bytes = sha1.digest();
        return String.format("%0" + (bytes.length << 1) + "x", new BigInteger(1, bytes));
    }
}

(This is not even compile-tested; I trust that you can get it to work, though.)

[ctrueden]

@Squareys See also org.scijava.util.DigestUtils, which will save you the effort of @dscho's example above.

[dscho]

D'oh. Thanks, @ctrueden, for setting my head straight.

[Squareys]

So, I need to compute the hash for the code input and then check, whether a entry for this hash already exists in the weak hash map.
The compile method accepts Reader, though. I could read through the Reader to extract a String of which I could compute the hash, but that doesn't seem like a particularly clean way to do this.
Any suggestions?

[dscho]

The compile method accepts Reader, though. I could read through the Reader to extract a String of which I could compute the hash, but that doesn't seem like a particularly clean way to do this.

That's what I referred to when I said this:

To this end, it might make sense to turn the logic around, from passing a StringReader to the compile() method that takes a Reader to passing a String to the other method.

[ctrueden]

@Squareys I wouldn't bother "prematurely optimizing" here. For the intended use cases, the String of source code will never be large enough to cause issues. If you go with Dscho's suggestion, it will make things easier.

[Squareys]

While I'm at it: Is there a Eclipse code formatter .xml somewhere I can use?

[dscho]

https://github.com/imagej/imagej/tree/master/config

[ctrueden]

@Squareys Please import the whole .epf at that link, since it contains settings beyond just Clean Up and Format. Specifically, it also has the order of import packages defined. Without doing that, we may get into "Clean-Up Wars."

[Squareys]

Thank you for mentioning that, I wouldn't have done so otherwise.

[Squareys]

Regarding avoiding recompilation; after trying to cram this in for a few hours, my oppinion on how to go about this changed:

I do not think, eval(..) nor compile(..) should support extra measures for avoiding recompilation. When compiling from a String or a Reader, a temporary maven project is created which is dependent on engine scope bindings (values assigned to "verbose" and "debug" keys). Therefore storing only the temporary directory in the hash map wont do it.

So, what if you still want to avoid recompilation? After having my problems understanding the compile(File, Writer) method, I realized, that this method merely compiles an existing file without doing all the other fancy stuff the compile(String/Reader) methods do. For this method to be useful, you will need to get the main class or whatever yourself. This also means the project files of the existing project probably won't change and minimaven can handle avoiding recompilation for multiple compile(File) calls (I am kinda assuming that for minimaven. Is this actually true?). My suggestion would be to improve the "workflow" around compile(File), maybe expose Builder after some more refactoring to provide simpler ways of getting the main class/running the project via JavaRunner etc, in another issue (This issue never meant to be about avoiding recompilation).

[ctrueden]

@Squareys Feel free to file issues along the lines of your thinking. FYI, realistically, no one at LOCI will be able to work on the scripting-java project any time soon. But you are more than welcome to make whatever improvements you need!

[ctrueden]

Regarding your questions, I am not an expert on the code. But personally, I would like to move away from any MiniMaven dependencies across the board. It is one more wrinkly component that we do not really have time to maintain. Rather, we should lean on the JRE's built-in infrastructure as much as possible, even if it means end users have to install a JDK. Maybe we could consolidate Java compilation logic into the scripting-java component itself, which can be reused in other places as needed? Unfortunately, I don't really have time to investigate and think through the issues thoroughly.

[Squareys]

Alright, I will merge my changes into master of my fork tomorrow, they will not contain recompilation avoidance (for wich I will not create a separate issue since I came to the conclusion that it is not necessary), but contains a few improvements to documentation, readability and consistency.

I will notify you after, when I believe this PR could be ready for merge.

[Squareys]

Alright. There is one last thing, which may or not be an issue: I switched up the order of execution from compile(String) -> compile(Reader) to compile(Reader) -> compile(String) which means reading the entire contents of the Reader into a String. Later, at the possible creation of the temp project, the String has been passed as StringReader and is read again to write out onto disk. (This was in preparation of the now cancelled/postponed compilation avoidance feature.)

The issue one could possibly see here is that this causes additional overhead when executing scripts from Readers. I don't believe this overhead is really significant though, I can't imagine reading from a StringReader to be too costly.

[Squareys]

@ctrueden if this is an issue for you, I will see what I can do, otherwise I declare this PR ready for merge from my side.

[Squareys]

@ctrueden I'm guessing time is sparse at the moment, just wanted to make sure you got the message that this is pull request is ready to be merged.

[ctrueden]

Thanks @Squareys. FWIW, 4913afd has failing tests:

testEvalReader(org.scijava.plugins.scripting.java.JavaEngineTest)  Time elapsed: 0.239 sec  <<< FAILURE!
java.lang.AssertionError: null
    at org.junit.Assert.fail(Assert.java:86)
    at org.junit.Assert.assertTrue(Assert.java:41)
    at org.junit.Assert.assertTrue(Assert.java:52)
    at org.scijava.plugins.scripting.java.JavaEngineTest.testEvalReader(JavaEngineTest.java:161)

And 040383e has unrelated formatting changes.

But really, those are nitpicks. I'm gonna merge it in the interest of time.