Progress API #12163
Progress API #12163
Conversation
JaroslavTulach
requested review from
hubertp,
4e6,
kazcw,
farmaazon and
Akirathan
JaroslavTulach
requested review from
jdunkerley,
radeusgd,
GregoryTravis,
AdRiley and
marthasharkey
as code owners
| var geom = ctx.eval("enso", code).invokeMember(MethodNames.Module.EVAL_EXPRESSION, "geom"); | ||
|
|
||
| var oneTimeLog = | ||
| TestLogger.apply( |
There was a problem hiding this comment.
To use this TestLogger class I created new logging-test-utils module in e80dfa0 and moved existing classes from logging-utils/src/test/ into it. That way we can have a regular JPMS dependency on the new module and we don't need to patch/trick the module system to exposes classes originally placed in test subdirectory.
| Progress.run "from 0 to "+n.to_text n progress-> | ||
| loop count_down = | ||
| if count_down <= 0 then combine.get else | ||
| combine.accept count_down progress |
There was a problem hiding this comment.
Report Progress from Java
The useExistingProgressFromJava test demonstrates how a Java code inside ofAccumulator.accept method can advance a progress that has previously been created in Enso code. Enough to accept the handle to progress as a Value argument and invokeMember("advance", 1) on it.
Such a call is crossing Java/Enso boundary. Is that fast? It is:
- fast for reporting progress of an HTTP download
- fast for reporting progress of executing
test/Base_Tests - not fast when reporting progress on every iteration of
1.up_to 100000 . map (+ 2)
2818c0e shows another approach using own typed interface instead of a generic Value.
There was a problem hiding this comment.
I assume the speed of typed interface is still relatively similar to the invokeMember, right?
Is the solution to just report the progress every n iterations (e.g. in methods like Table::join)?
There was a problem hiding this comment.
the speed of typed interface is still relatively similar to the
invokeMember, right?
Yes, very likely it is identical.
not fast when reporting progress on every iteration of 1.up_to 100000 . map (+ 2)
Is the solution to just report the progress every n iterations (e.g. in methods like Table::join)?
How fast you want the progress reporting to be? With the proposal we have right now the performance is going to be similar to logging and I doubt you can do:
var res = new int[arr.length];
for (var i = 0; i < arr.length; i++) {
res[i] = arr[i] + 2;
logger.info("Just added two to " + arr[i] + " yielding " + res[i]);
}and assume the performance will not be affected!
Define Performance Goals
We should probably set some expectations up:
- how fast it should be to
advanceby default (e.g.1) - how fast it should be to
advanceby more - how fast it should be to
loga detailed message about the progress - how fast it should be from Enso?
- how fast it should be from Java?
- is it OK to require more complicated code in Java to reach "zero overhead" progress reporting?
Only then we can measure if we are fast enough.
...time-integration-tests/src/test/java/org/enso/interpreter/runtime/progress/ProgressTest.java
Show resolved
Hide resolved
Co-authored-by: Radosław Waśko <[email protected]>
| * Example of a progress aggregator able to nest multiple Progress and compute % of aggregated | ||
| * progress. | ||
| */ | ||
| public final class ProgressAggregator { |
There was a problem hiding this comment.
Aggregation of Nested Progress Instances
An essential concept this Progress API offers is aggregation of nested Progress instances into a single percentage number. The general idea is: language server instrument will create new ProgressAggregator when new node computation is being onEntered:
it will consume all the requests for Progress creation and their advance or close calls. Based on that it will compute % of computation of the node state and report it to the IDE using Pending.progress: number?.
The algorithm should look like the one demonstrated in the ProgressAggregator class. E.g.:
- each
Progressis created with a finite amount of steps- immediately step zero is assumed to be in progress
- e.g. we are working on interval
0.0-1.0/max - once the code
advanceby1, we report1.0/maxpercentage is finished - and start working on
1.0/maxto2.0/maxinterval
- when a nested progress is created with
max2steps- we split the current interval into
max2intervals - we let the nested progress to work on these intervals only
- e.g. never crossing the current step interval of parent
Progress
- we split the current interval into
- when a
Progressis closed - in Enso: when itsactionis over- it reports 100% own status
- whole its interval is reported as finished
- the progress monotonically increases - it shall never decrease
|
|
||
| advance self amount:Integer -> Progress = | ||
| add = if amount>=0 then amount else 0 | ||
| self.log_message self.label+"+"+add.to_text Log_Level.Finest # TBD |
There was a problem hiding this comment.
I don't like that we use special characters as delimiters. Instead of using strings, the Progress class can emit more structured events
There was a problem hiding this comment.
Yes, you are right. Emitting and parsing text does not form the best (internal) API.
This is not the final state of the internals yet (note the # TBD comment at the end of line). My plan is to use Structured Logging for Communication just like we did in NetBeans.
However from the point of Enso Progress API, it is not really important how exactly we deliver the information to language server - thus I asked for inception review already with this detail remaining to be done.
| } | ||
|
|
||
| public static final class AccumulatorWithProgress { | ||
| // implements BiConsumer<Long, Progress>, Supplier<Long> { |
There was a problem hiding this comment.
I tried to be type safe in 08497fb#diff-6bf51b15bc0a5c0977d3598560cda370dd47d1a02454122b2489016f56513036R139, but then I found such an approach cannot be expanded to 2818c0e because of generic type erasure (Truffle sees value of Object and not of Progress or Value) and had to comment the implements out. Now Truffle find the real method signatures via reflection and uses them directly.
Pull Request Description
Defines Progress API - a way to start long running actions and to report their progress.
Important Notes
The "Enso facing API" consists of
type Progresswith its static method to execute a progress reporting operations. The operation is a function that getsProgressinstance as a callback and can use its methods to report theadvanceas well aslogdetails of the actual operation in progress.This is all very similar to logging:
Progresstype is placed into loggingPlease review this initial concept. If it is found OK, we can move towards collecting the information in language server and delivering them to the IDE to display the progress via #12126 & co.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Prior work
Progress.runProgress.runinto a callback avoids stalled progress actionsactionis over, the progress is automatically reported as finished: 59d63bbProgress.runinvocations are allowedIllegalStateException