[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-openjfx-dev
Subject: Next steps ? (Re: An attempt of a CSR draft ... (Re: A new WIP (PR # 192) (Re: WIP version with PI c
From: "Rony G. Flatscher" <Rony.Flatscher () wu ! ac ! at>
Date: 2020-04-28 13:15:54
Message-ID: 34d30efa-a87c-261e-36b3-e5f68f38c389 () wu ! ac ! at
[Download RAW message or body]
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)).
> > > > > >
> > > > > >
> > > > > > ---rony
> > > > > >
> > > > > >
> > > > > >
> > > > > > > On 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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic