-- Kevin
On 5/23/2020 11:30 AM, Rony G Flatscher wrote:
Hi Kevin, is there anything I need to do? What are the next steps I should look for? —-rony Rony G. Flatscher (mobil/e)Am 12.05.2020 um 19:23 schrieb Rony G. Flatscher <rony.flatsc...@wu.ac.at>: Hi Kevin, in the meantime I have tried to come up with a formulation for the "Introduction to FXML" specification about the new compile processing instruction which is brief and complete. While being there I fixed a typo in the document and added a missing language processing instruction to an existing script example, hope that was o.k. as I have not filed an explicit bug for it. Also closed the PR 129 and 187, removed the WIP from PR 192, and supplied the CSR draft as a comment. If there is anything else I should do, please let me know. Cheers, ---ronyOn 11.05.2020 14:12, Rony G. Flatscher wrote: Hi Kevin,On 09.05.2020 17:16, Kevin Rushforth wrote: I'm finally getting back to this. I took a look at https://github.com/openjdk/jfx/pull/192 and I like that as the direction for this enhancement. The initial CSR you have is a good start.Thank you!Next steps are: 1. Update the "Introduction to FXML" specification (see my comment in the PR) 2. Update PR 192 with the draft CSR as a comment, modifying it to include the above additions to "Introduction to FXML" 3. Remove WIP from the title You can then close the other two PRs (129 and 187).will do (may take a little bit). Cheers ---ronyOn 4/28/2020 6:15 AM, Rony G. Flatscher wrote:Hi Kevin, what should be the next steps? Should I remove "WIP" from the title in <https://github.com/openjdk/jfx/pull/192> and add the CSR draft text of my last e-mail as a "CSR" comment with PR # 192, thereby requesting it to be added to <https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>? Please advise. TIA, ---rony P.S.: This is the RFE: * RFE (2020-01-24): <https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080> These are the three versions (all with appropriate unit tests) that I came up with chronologically to implement the RFE, would prefer the latest version (PR # 192): * Compile if Compilable implemented (2020-02-28): <https://github.com/openjdk/jfx/pull/129> * Compile if compile PI and Compilable is implemented (2020-04-11): <https://github.com/openjdk/jfx/pull/187> * Compile with fallback, if Compilable is implemented, compile PI for fine-grained control (2020-04-14): <https://github.com/openjdk/jfx/pull/192> On 22.04.2020 20:01, Rony G. Flatscher wrote:Hi Kevin, as I am not able to file a CSR with the issue you suggested to come up with a draft, so here it goes: Summary ======= Have javafx.fxml.FXMLLoader compile FXML scripts before evaluating them, if the script engine implements the javax.script.Compilable interface to speed up execution. In case compilation throws a javax.script.ScriptException fall back to evaluating the uncompiled script. Allow control of script compilation with a "compile" PI for FXML files. Problem ======= javafx.fxml.FXMLLoader is able to execute scripts in Java script languages (javax.script.ScriptEngine implementations) referred to or embedded in a FXML file. If a script engine implements the javax.script.Compilable interface, then such scripts could be compiled and the resulting javax.script.CompiledScript could be executed instead using its eval() methods. Evaluating the javax.script.CompiledScript objects may help speed up the execution of script invocations, especially for scripts defined for event attributes in FXML elements (e.g. like onMouseMove) which may be repetitively invoked and evaluated. Solution ======== Before evaluating the script code test whether the javax.script.ScriptEngine implements javax.script.Compilable. If so, compile the script to a javax.script.CompiledScript object first and then use its eval() method to evaluate the script, otherwise continue to use the javax.script.ScriptEngine's eval() method instead. Should compilation of a script yield (unexpectedly) a javax.script.ScriptException then fall back to using the javax.script.ScriptEngine's eval() method. A new process instruction "compile" allows control of the compilation of scripts ("true" sets compilation on, "false" to set compilation off) in FXML files. Specification ============= If a javax.script.ScriptEngine implements the javax.script.Compilable interface, then use its compile() method to compile the script to a javax.script.CompiledScript object and use its eval() method to run the script. In the case that the compilation throws (unexpectedly) a javax.script.ScriptException log a warning and fall back to using the javax.script.ScriptEngine's eval() method instead. To allow setting this feature off and on while processing the FXML file a "compile" process instruction ("<?compile false?>" or "<?compile true?>") gets defined that allows to turn compilation off and on throughout a FXML file. Having never seen a real CSR I hope that this matches what is expected and is helpful for assessment. If not please advise (got the name of these fields from [1]). --- Also added brief information about the respective test units (what they test and yield) in the WIP [2]. ---rony [1] "CSR-FAQ": <https://wiki.openjdk.java.net/display/csr/CSR+FAQs> [2] "WIP: Script compilable+compile PI+fallback: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts #192": <https://github.com/openjdk/jfx/pull/192> On 20.04.2020 14:58, Rony G. Flatscher wrote:There is a new WIP at <https://github.com/openjdk/jfx/pull/192>: This WIP adds the ability for a fallback in case compilation of scripts fails, in which case a warning gets issued about this fact and evaluation of the script will be done without compilation. Because of the fallback scripts get compiled with this version by default. It extends PR 187 #187. To further ease spotting scripts that cause a ScriptException a message in the form of "filename: caused ScriptException" gets added to the exception handling in either of the three locations: an error message, a stack trace or a wrap-up into a RuntimeException (having three different kinds of reporting ScriptExceptions may be questioned, however none of these tear down the FXML GUI). This WIP comes with proper test units as well. As per Kevin's suggestion a warning gets logged whenever a script cannot be compiled and the fallback gets used. It is suggested to use this WIP as it includes the compilation by default with a safe fallback to evaluate the uncompiled script, if compilation (unexpectedly) fails. Again, any feedback, discussion welcome! ---rony P.S.: In the log history there is a commit message "Make message more pregnant.", it should have read "Make messages more terse." instead|.|| | On 17.04.2020 19:37, Rony G. Flatscher wrote:There is a new WIP at <https://github.com/openjdk/jfx/pull/187> which adds a compile PI (process instruction) for turning on and off script compilation if the script engine implements the Compilable interface. By default compilation is off (no compilation), such that one needs to add a compile PI ("<?compile?>") at the top to activate this feature. Supplying "true" (default) or "false" as the PI data turns this feature on and off. The WIP comes with adapted test units that test "compile on" for an entire fxml file, "compile off", alternating using "compile on and off", and alternating using "compile off and on". This will test all variants of applying the compile PI for all categories of scripts. Any feedback appreciated! ---rony P.S.: FXML files that contain unknown PIs do not cause a runtime error by FXMLLoader, they just get ignored. Therefore one could apply the compile PI to FXML files that are used in older JavaFX runtimes. P.P.S.: In the next days I will also add Kevin's idea in a separate version that will have a fallback solution in case a compilation is (unexpectedly) not successful, reverting to (interpretative) evaluation/execution of the script. In that version it is planned to have compilation on by default as in the case of a compilation failure there will be a safe backup solution. On 14.04.2020 19:52, Kevin Rushforth wrote:Yes, I agree that enough time has gone by. Go ahead with your proposal. I would wait a bit to create the CSR until the review is far enough along to know which direction we intend to go. Unless there is a real concern about possible regressions if scripts are compiled by default, I think "enabled by default" is the way to go. Your argument that such script engines are broken seems reasonable, since this only applies to script engines that implement javax.script.Compilable in the first place. We still might want to add way to turn compilation off for individual scripts. One other thing to consider is that if compilation fails, it might make sense to log a warning and fall back to the existing interpreted mode. Does anyone else have any concerns with this? -- Kevin On 4/14/2020 9:48 AM, Rony G. Flatscher wrote:Hi there, as there was probably enough time that has passed by I would intend to create a CSR in the next days with the PR as per Kevin's suggestion. (For the case that this feature should not be active by default, the CSR will suggest to define a new "compile" PI in the form <?compile [true|false] ?> (default, if no PI data given: true), which is independent of the existence of a language PI (this way it becomes also possible to allow compilation of external scripts denoted with the script-element, which do not need a page language to be set as the file's extension allows the appropriate script engine to be loaded and used for execution). A compile-PI would allow for turning compilation of scripts on by just adding the PI <?compile?> or <?compile true?> to FXML files (and <?compile false?> to turn off), which seems to be simple and self-documentary. In general employing such compile PIs allows for setting compilation of scripts on and off throughout an FXML file.) ---rony On 04.04.2020 18:03, Rony G. Flatscher wrote:Hi Kevin, On 03.04.2020 01:21, Kevin Rushforth wrote:I see that you updated the PR and sent it for review. Before we formally review it in the PR, let's finish the discussion as to whether this is a useful feature, and if so, what form this feature should take. From my point of view, this does seem like a useful feature. Would other users of FXML benefit from it?Script code should be executed faster after compilation, so any FXML page that hosts script code may benefit. The benefits depend on the type of script (and maybe its size and its complexity) and also on the types of event handlers the scripts serve, e.g. move or drag event handlers may benefit significantly. This is because repeated invocation of compiled script event handlers do not cause the reparsing of that script's source and interpreting it on each invocation, which may be expensive depending on the script engine, but rather allows the immediate evaluation/execution of the compiled script by the script engine.I'm not certain whether we want it to be implicit, compiling the script if the script engine in question implements Compilable, or via a new keyword or tag. What are the pros / cons?In principle there are three possibilities: 1) If a script engine implements javax.script.Compilable, compile the script and execute the compiled version. In the case of event handlers compile and buffer the compiled script and execute the compiled script each time its registered event fires. o Pro: immediately benefits all existing FXML pages that host scripts o Con: it is theoretically possible (albeit quite unlikely) that there are scripts that fail compiling but have been employed successfully in interpreted mode 2) Introduce some form of an optional attribute (e.g. "compile={true|false}") to the FXML language PI that switches on compilation of scripts hosted in FXML definitions if the script engine implements the javax.script.Compilable interface. If missing it would default to "false". (Alternatively, add a "compile" PI, that if present causes the compilation of scripts, if the script engine supports it. It would be an error if the "compile" PI was present, but the "language" PI was not.) o Pro: compilation of FXML hosted scripts is done only, if the FXML definition of the language PI gets changed o Con: benefit not made available automatically to existing FXML pages that host scripts 3) Another possibility would be to define a boolean attribute/property "compile" for script and node elements and only compile the scripts, if the property is set to true o Pro: compilation of FXML hosted scripts is done only, if the FXML definition gets changed accordingly o Con: potential benefit not made available automatically to existing FXML pages that host scripts 2 and 3 could be combined, where 2 would define the default compilation behavior that then could be overruled individually by 3. The question would be whether 2 or/and 3 are really necessary as it can be expected that compilation of scripts by the script engines would find the same errors as while interpreting the very same scripts (if not, the script engine is badly broken and it could be argued that then the interpretation part of the script engine might be expected to be broken as well which would be quite dangerous from an integrity POV; the same consideration applies as well if the execution of the compiled script would behave differently to interpreting the very same script by the same script engine). The current WIP implements 1 above and includes an appropriate test unit.What do others think? In either case, we would need to make sure that this doesn't present any security concerns in the presence of a security manager. As long as a user-provided class is on the stack, it will be fine, but we would need to ensure that.The compilation of scripts needs to be done by the Java script engines (implementing both, javax.script.Engine and javax.script.Compilable) as well as controlling the execution of compiled scripts ([javax.script.CompiledScript] (https://docs.oracle.com/en/java/javase/14/docs/api/java.scripting/javax/script/CompiledScript.html)). ---ronyOn 4/2/2020 10:41 AM, Rony G. Flatscher wrote:After merging master, applying some fixes and changing the title to reflect the change from WIP to a pull request I would kindly request a review of this pull request! Here the URL: <https://github.com/openjdk/jfx/pull/129>, title changed to: "8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts". ---rony On 28.02.2020 19:22, Rony G. Flatscher wrote:Here is a WIP [1] implementation of [2]. The WIP is based on [3], which is currently in review, and has an appropriate test unit going with it as well, please review. There should be no compatibility issue with this implementation. Discussion: another solution could be to not compile by default. Rather compile, if some new information is present with the FXML file which cannot possibly be present in existing FXML files. In this scenario one possible and probably simple solution would be to only compile scripts if the language process instruction (e.g. <?language rexx?>) contains an appropriate attribute with a value indicating that compilation should be carried out (e.g.: compile="true"). This way only FXML with PIs having this attribute set to true would be affected. If desired I could try to come up with a respective solution as well. ---rony [1] "Implementation and test unit": <https://github.com/openjdk/jfx/pull/129> [2] "JDK-8238080 : FXMLLoader: if script engines implement javax.script.Compilable compile scripts": <https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080> [3] "8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV": <https://github.com/openjdk/jfx/pull/122/commits> On 24.01.2020 16:26, Kevin Rushforth wrote:Thank you for filing this enhancement request. As an enhancement it should be discussed on this list before proceeding with a pull request (although a "WIP" or Draft PR can be used to illustrate the concept). For my part, this seems like a reasonable enhancement, as long as there are no compatibility issues, but it would be good to hear from application developers who heavily use FXML. -- Kevin On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:Just filed a RFE with the following information: * Component: o JavaFX * Subcomponent: o fxml: JavaFX FXML * Synopsis: o "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts" * Descriptions: o "FXMLLoader is able to execute scripts in Java script languages (javax.script.ScriptEngine implementations) if such a Java script language gets defined as the controller language in the FXML file. If a script engine implements the javax.script.Compilable interface, then such scripts could be compiled and the resulting javax.script.CompiledScript could be executed instead using its eval() methods. Evaluating the CompiledScript objects may help speed up the execution of script invocations, especially for scripts defined for event attributes in FXML elements (e.g. like onMouseMove) which may be repetitevly invoked and evaluated." * System /OS/Java Runtime Information: o "All systems." Received the internal review ID: "9063426" ---rony
