Amortize slow LFF startup time #1788
Conversation
| private static Writer writer; | ||
|
|
||
| private static BufferedReader reader; | ||
| private static BufferedReader error; |
There was a problem hiding this comment.
It is a little bit of a red flag that these variables are static. It is necessary because Step needs to be Serializable. I do not fully understand why this is required by Spotless, but I am under the impression that it relates to an optimization that we are not trying to use at the moment.
petervdonovan
force-pushed
the
slow-lff
branch
2 times, most recently
from
83afd7f to
c142bf4
Compare
There was a problem hiding this comment.
Great! I changed the base to the gradle branch. It's probably best to merge it there first.
There is one major issue with this though. As lff runs as a daemon in the background, any changes we make to the formatter do not have an effect until we kill the gradle daemon and then do a clean build. Can we just spawn a new lff process when create is called and kill the old one?
|
Since you are the first to work with the new repo structure in #1779. Do you think it makes sense? |
Yes, I think so. One minor issue that I see is that |
|
On line 26, where it says spotlessLinguaFranca.dependsOn(":org.lflang:cli:lff:installDist")There seems to be a circular dependency, for running which must mean that we ran EDIT: RESOLVED. See commit message of fe8dfb9 for details. |
This terminates the spawned formatter by closing its input stream. The formatter should detect that the stream is closed and understand that it will not receive any more files to format and exit gracefully. This prevents the formatter from outliving its Gradle daemon. If the daemon is terminated forcefully e.g. with a -9 SIGTERM rather than a -15 SIGKILL, then this will not work. Otherwise I believe that this should release the resource (get rid of the spawned formatter) correctly.
The problem was not that Gradle runs any spotlessLinguaFranca tasks as part of its other operations, but that it configures the task before starting any other operation (including clean). The solution is to start the formatter lazily -- to wait until spotlessLinguaFranca actually operates on a file before trying to start up the formatter process.
Yes, good idea. Now we always run |
This is just an artifact of the porting process. The plan is to eliminate |
This ensures, that gradle recognizes a change in lff and reformats all lf files accordingly.
There was a problem hiding this comment.
This looks great! I think the static variables and the fact that we need to run a separate lff daemon, are just artifacts of the hacky thing we as doing, where a build flow (formatting) also depends on our build output (lff). It seems to work fine though!
I made a small change and added the outputs of the task that insalls lff to the inputs of our formatting tasks. This ensures that gradle automatically reformats all lf files when lff changed.
Fixes #1342.
Benefits of correctly incorporating LFF into Spotless include:
This solution works by giving the formatter the ability to listen in its input stream for the paths that it needs to format. This lets us use one instance of the formatter for the entire lifetime of the calling Gradle daemon.
To my knowledge, Spotless does not provide an API for releasing resources used by the formatting step across multiple invocations of the formatter. However the JVM does provide a shutdown hook, which I think is an acceptable though imperfect way to release the formatter. I have verified on Ubuntu that the formatter subprocess does indeed terminate when the enclosing Gradle daemon is killed with a SIGKILL.