change the recurse interface to AbstractInterpreter-like interface
#683
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aviatesk
added a commit
to JuliaDebug/LoweredCodeUtils.jl
that referenced
this pull request
Apr 21, 2025
This commit implements the migration to the new JuliaInterpreter interface proposed in JuliaDebug/JuliaInterpreter.jl#683. It purely performs the migration to the new interface and does not include any refactoring based on it. `selective_eval!` could now be rewritten as follows using the new interface, that change is not made in this commit for minimizing the diff: ```diff diff --git a/src/codeedges.jl b/src/codeedges.jl index 3cf2a17..5eba604 100644 --- a/src/codeedges.jl +++ b/src/codeedges.jl @@ -1021,6 +1021,33 @@ function add_inplace!(isrequired, src, edges, norequire) return changed end +struct SelectiveInterpreter{S<:Interpreter,T<:AbstractVector{Bool}} <: Interpreter + inner::S + isrequired::T +end +function JuliaInterpreter.step_expr!(interp::SelectiveInterpreter, frame::Frame, istoplevel::Bool) + pc = frame.pc + if interp.isrequired[pc] + step_expr!(interp.inner, frame::Frame, istoplevel::Bool) + else + next_or_nothing!(interp, frame) + end +end +function JuliaInterpreter.get_return(interp::SelectiveInterpreter, frame::Frame) + pc = frame.pc + node = pc_expr(frame, pc) + if is_return(node) + if interp.isrequired[pc] + return lookup_return(frame, node) + end + else + if isassigned(frame.framedata.ssavalues, pc) + return frame.framedata.ssavalues[pcexec] + end + end + return nothing +end + """ selective_eval!([interp::Interpreter=RecursiveInterpreter()], frame::Frame, isrequired::AbstractVector{Bool}, istoplevel=false) @@ -1037,27 +1064,10 @@ This will return either a `BreakpointRef`, the value obtained from the last exec Typically, assignment to a variable binding does not result in an ssa store by JuliaInterpreter. """ function selective_eval!(interp::Interpreter, frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) - pc = pcexec = pclast = frame.pc - while isa(pc, Int) - frame.pc = pc - pclast = pcexec::Int - if isrequired[pc] - pcexec = pc = step_expr!(interp, frame, istoplevel) - else - pc = next_or_nothing!(interp, frame) - end - end - isa(pc, BreakpointRef) && return pc - pcexec = (pcexec === nothing ? pclast : pcexec)::Int - frame.pc = pcexec - node = pc_expr(frame) - is_return(node) && return isrequired[pcexec] ? lookup_return(frame, node) : nothing - isassigned(frame.framedata.ssavalues, pcexec) && return frame.framedata.ssavalues[pcexec] - return nothing + return JuliaInterpreter.finish_and_return!(SelectiveInterpreter(interp, isrequired), frame, istoplevel) end -function selective_eval!(frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) +selective_eval!(frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) = selective_eval!(RecursiveInterpreter(), frame, isrequired, istoplevel) -end """ selective_eval_fromstart!([interp::Interpreter=RecursiveInterpreter()], frame, isrequired, istoplevel=false) ```
aviatesk
force-pushed
the
avi/interp-interface
branch
2 times, most recently
from
d26f140 to
8a12ee5
Compare
Member
Author
|
If I have one concern about this PR, which is that the change from the old argument type declaration However it might be positive, as |
aviatesk
added a commit
to timholy/Revise.jl
that referenced
this pull request
Apr 21, 2025
This commit implements the migration to the new JuliaInterpreter interface proposed in JuliaDebug/JuliaInterpreter.jl#683. It purely performs the migration to the new interface and does not include any refactoring based on it. In practice, `methods_by_execution!` is quite complex, and using the `Interpreter` interface may not allow us to simplify its implementation. That said, this commit seems to achieve a modest latency improvement by changing the argument type declaration from `@nospecialize(recurse)` to `interp::Interpreter` (with easier code specialization).
Member
Author
|
I tried using Revise on Example.jl with timholy/Revise.jl#911 locally, and it seems like it have slightly improved the latency. I'm not sure how to accurately measure the latency of the First-Time-To-Revise, so it could be just a margin of error. |
aviatesk
force-pushed
the
avi/interp-interface
branch
from
8a12ee5 to
7b6ca0d
Compare
This was referenced Apr 21, 2025
aviatesk
force-pushed
the
avi/interp-interface
branch
from
d662e72 to
b6d255c
Compare
Align the `recurse` argument to something like the base Compiler's `AbstractInterpreter` and make JuliaInterpreter routines overloadable properly. This change is quite breaking (thus bumping the minor version of this package), but necessary to enhance the customizability of JI. For example, it will make it easier to add changes like #682 in a nicer way, but also should enable better designs in packages such as Revise and JET.
aviatesk
force-pushed
the
avi/interp-interface
branch
from
b6d255c to
eaf72f0
Compare
aviatesk
added a commit
to JuliaDebug/LoweredCodeUtils.jl
that referenced
this pull request
Apr 22, 2025
This commit implements the migration to the new JuliaInterpreter interface proposed in JuliaDebug/JuliaInterpreter.jl#683. It purely performs the migration to the new interface and does not include any refactoring based on it. `selective_eval!` could now be rewritten as follows using the new interface, that change is not made in this commit for minimizing the diff: ```diff diff --git a/src/codeedges.jl b/src/codeedges.jl index 3cf2a17..5eba604 100644 --- a/src/codeedges.jl +++ b/src/codeedges.jl @@ -1021,6 +1021,33 @@ function add_inplace!(isrequired, src, edges, norequire) return changed end +struct SelectiveInterpreter{S<:Interpreter,T<:AbstractVector{Bool}} <: Interpreter + inner::S + isrequired::T +end +function JuliaInterpreter.step_expr!(interp::SelectiveInterpreter, frame::Frame, istoplevel::Bool) + pc = frame.pc + if interp.isrequired[pc] + step_expr!(interp.inner, frame::Frame, istoplevel::Bool) + else + next_or_nothing!(interp, frame) + end +end +function JuliaInterpreter.get_return(interp::SelectiveInterpreter, frame::Frame) + pc = frame.pc + node = pc_expr(frame, pc) + if is_return(node) + if interp.isrequired[pc] + return lookup_return(frame, node) + end + else + if isassigned(frame.framedata.ssavalues, pc) + return frame.framedata.ssavalues[pcexec] + end + end + return nothing +end + """ selective_eval!([interp::Interpreter=RecursiveInterpreter()], frame::Frame, isrequired::AbstractVector{Bool}, istoplevel=false) @@ -1037,27 +1064,10 @@ This will return either a `BreakpointRef`, the value obtained from the last exec Typically, assignment to a variable binding does not result in an ssa store by JuliaInterpreter. """ function selective_eval!(interp::Interpreter, frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) - pc = pcexec = pclast = frame.pc - while isa(pc, Int) - frame.pc = pc - pclast = pcexec::Int - if isrequired[pc] - pcexec = pc = step_expr!(interp, frame, istoplevel) - else - pc = next_or_nothing!(interp, frame) - end - end - isa(pc, BreakpointRef) && return pc - pcexec = (pcexec === nothing ? pclast : pcexec)::Int - frame.pc = pcexec - node = pc_expr(frame) - is_return(node) && return isrequired[pcexec] ? lookup_return(frame, node) : nothing - isassigned(frame.framedata.ssavalues, pcexec) && return frame.framedata.ssavalues[pcexec] - return nothing + return JuliaInterpreter.finish_and_return!(SelectiveInterpreter(interp, isrequired), frame, istoplevel) end -function selective_eval!(frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) +selective_eval!(frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) = selective_eval!(RecursiveInterpreter(), frame, isrequired, istoplevel) -end """ selective_eval_fromstart!([interp::Interpreter=RecursiveInterpreter()], frame, isrequired, istoplevel=false) ```
aviatesk
added a commit
to JuliaDebug/LoweredCodeUtils.jl
that referenced
this pull request
Apr 22, 2025
This commit implements the migration to the new JuliaInterpreter interface proposed in JuliaDebug/JuliaInterpreter.jl#683. It purely performs the migration to the new interface and does not include any refactoring based on it. `selective_eval!` could now be rewritten as follows using the new interface, that change is not made in this commit for minimizing the diff: ```diff diff --git a/src/codeedges.jl b/src/codeedges.jl index 3cf2a17..5eba604 100644 --- a/src/codeedges.jl +++ b/src/codeedges.jl @@ -1021,6 +1021,33 @@ function add_inplace!(isrequired, src, edges, norequire) return changed end +struct SelectiveInterpreter{S<:Interpreter,T<:AbstractVector{Bool}} <: Interpreter + inner::S + isrequired::T +end +function JuliaInterpreter.step_expr!(interp::SelectiveInterpreter, frame::Frame, istoplevel::Bool) + pc = frame.pc + if interp.isrequired[pc] + step_expr!(interp.inner, frame::Frame, istoplevel::Bool) + else + next_or_nothing!(interp, frame) + end +end +function JuliaInterpreter.get_return(interp::SelectiveInterpreter, frame::Frame) + pc = frame.pc + node = pc_expr(frame, pc) + if is_return(node) + if interp.isrequired[pc] + return lookup_return(frame, node) + end + else + if isassigned(frame.framedata.ssavalues, pc) + return frame.framedata.ssavalues[pcexec] + end + end + return nothing +end + """ selective_eval!([interp::Interpreter=RecursiveInterpreter()], frame::Frame, isrequired::AbstractVector{Bool}, istoplevel=false) @@ -1037,27 +1064,10 @@ This will return either a `BreakpointRef`, the value obtained from the last exec Typically, assignment to a variable binding does not result in an ssa store by JuliaInterpreter. """ function selective_eval!(interp::Interpreter, frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) - pc = pcexec = pclast = frame.pc - while isa(pc, Int) - frame.pc = pc - pclast = pcexec::Int - if isrequired[pc] - pcexec = pc = step_expr!(interp, frame, istoplevel) - else - pc = next_or_nothing!(interp, frame) - end - end - isa(pc, BreakpointRef) && return pc - pcexec = (pcexec === nothing ? pclast : pcexec)::Int - frame.pc = pcexec - node = pc_expr(frame) - is_return(node) && return isrequired[pcexec] ? lookup_return(frame, node) : nothing - isassigned(frame.framedata.ssavalues, pcexec) && return frame.framedata.ssavalues[pcexec] - return nothing + return JuliaInterpreter.finish_and_return!(SelectiveInterpreter(interp, isrequired), frame, istoplevel) end -function selective_eval!(frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) +selective_eval!(frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) = selective_eval!(RecursiveInterpreter(), frame, isrequired, istoplevel) -end """ selective_eval_fromstart!([interp::Interpreter=RecursiveInterpreter()], frame, isrequired, istoplevel=false) ```
Member
Author
|
This PR is pretty big, but I've been using it locally since yesterday and haven't run into any issues. So, if there are no major objections, I plan to merge it tomorrow. @timholy Sorry to ping you again, but if you have a moment, it would be great to get your thoughts, even just on the general idea. Regarding the main concern about the impact on Revise's latency, this PR (and the related Revise PR) actually seems to improve latency slightly. So, I think we're okay to move forward with this direction regarding this aspect. |
aviatesk
added a commit
to timholy/Revise.jl
that referenced
this pull request
Apr 22, 2025
This commit implements the migration to the new JuliaInterpreter interface proposed in JuliaDebug/JuliaInterpreter.jl#683. It purely performs the migration to the new interface and does not include any refactoring based on it. In practice, `methods_by_execution!` is quite complex, and using the `Interpreter` interface may not allow us to simplify its implementation. That said, this commit seems to achieve a modest latency improvement by changing the argument type declaration from `@nospecialize(recurse)` to `interp::Interpreter` (with easier code specialization).
`Compiled` is preserved for backward compatibility.
serenity4
approved these changes
Apr 22, 2025
Collaborator
serenity4
left a comment
serenity4
left a comment
There was a problem hiding this comment.
That's a very nice refactor! The type annotations are also very welcome.
I made a few (very minor) suggestions, but I think the design is great. There remain uses of Compiled in the codebase, perhaps those should be replaced to NonRecursiveInterpreter so we only ever use Compiled for defining the alias and as export (but never use that name internally)?
Co-authored-by: Cédric Belmant <[email protected]>
Member
Author
I reviewed the places where |
aviatesk
force-pushed
the
avi/interp-interface
branch
from
7f708ce to
82e9948
Compare
Member
Author
|
@serenity4 Thanks for your reviews! Very much appreciated. |
aviatesk
added a commit
to JuliaDebug/LoweredCodeUtils.jl
that referenced
this pull request
Apr 23, 2025
* update to JuliaInterpreter 0.10 This commit implements the migration to the new JuliaInterpreter interface proposed in JuliaDebug/JuliaInterpreter.jl#683. It purely performs the migration to the new interface and does not include any refactoring based on it. `selective_eval!` could now be rewritten as follows using the new interface, that change is not made in this commit for minimizing the diff: ```diff diff --git a/src/codeedges.jl b/src/codeedges.jl index 3cf2a17..5eba604 100644 --- a/src/codeedges.jl +++ b/src/codeedges.jl @@ -1021,6 +1021,33 @@ function add_inplace!(isrequired, src, edges, norequire) return changed end +struct SelectiveInterpreter{S<:Interpreter,T<:AbstractVector{Bool}} <: Interpreter + inner::S + isrequired::T +end +function JuliaInterpreter.step_expr!(interp::SelectiveInterpreter, frame::Frame, istoplevel::Bool) + pc = frame.pc + if interp.isrequired[pc] + step_expr!(interp.inner, frame::Frame, istoplevel::Bool) + else + next_or_nothing!(interp, frame) + end +end +function JuliaInterpreter.get_return(interp::SelectiveInterpreter, frame::Frame) + pc = frame.pc + node = pc_expr(frame, pc) + if is_return(node) + if interp.isrequired[pc] + return lookup_return(frame, node) + end + else + if isassigned(frame.framedata.ssavalues, pc) + return frame.framedata.ssavalues[pcexec] + end + end + return nothing +end + """ selective_eval!([interp::Interpreter=RecursiveInterpreter()], frame::Frame, isrequired::AbstractVector{Bool}, istoplevel=false) @@ -1037,27 +1064,10 @@ This will return either a `BreakpointRef`, the value obtained from the last exec Typically, assignment to a variable binding does not result in an ssa store by JuliaInterpreter. """ function selective_eval!(interp::Interpreter, frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) - pc = pcexec = pclast = frame.pc - while isa(pc, Int) - frame.pc = pc - pclast = pcexec::Int - if isrequired[pc] - pcexec = pc = step_expr!(interp, frame, istoplevel) - else - pc = next_or_nothing!(interp, frame) - end - end - isa(pc, BreakpointRef) && return pc - pcexec = (pcexec === nothing ? pclast : pcexec)::Int - frame.pc = pcexec - node = pc_expr(frame) - is_return(node) && return isrequired[pcexec] ? lookup_return(frame, node) : nothing - isassigned(frame.framedata.ssavalues, pcexec) && return frame.framedata.ssavalues[pcexec] - return nothing + return JuliaInterpreter.finish_and_return!(SelectiveInterpreter(interp, isrequired), frame, istoplevel) end -function selective_eval!(frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) +selective_eval!(frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) = selective_eval!(RecursiveInterpreter(), frame, isrequired, istoplevel) -end """ selective_eval_fromstart!([interp::Interpreter=RecursiveInterpreter()], frame, isrequired, istoplevel=false) ``` * a bit better `selective_eval` * rm no longer used `finish_and_return!` import * propagate `interp` context to `lookup` * fix docs
aviatesk
added a commit
to timholy/Revise.jl
that referenced
this pull request
Apr 23, 2025
* update to JuliaInterpreter 0.10 This commit implements the migration to the new JuliaInterpreter interface proposed in JuliaDebug/JuliaInterpreter.jl#683. It purely performs the migration to the new interface and does not include any refactoring based on it. In practice, `methods_by_execution!` is quite complex, and using the `Interpreter` interface may not allow us to simplify its implementation. That said, this commit seems to achieve a modest latency improvement by changing the argument type declaration from `@nospecialize(recurse)` to `interp::Interpreter` (with easier code specialization). * checking in docs/Manifest.toml
Member
|
Sorry I missed this before merging! I'm very supportive of this general idea. With the hopes that JuliaLowering will become a thing in the forseeable future, Revise probably won't even need to depend on JuliaInterpreter forever. So things that advance the state of the art in JuliaInterpreter should not be held back. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Align the
recurseargument to something like the base Compiler'sAbstractInterpreterand make JuliaInterpreter routines overloadable properly.This change is quite breaking (thus bumping the minor version of this package), but necessary to enhance the customizability of JI. For example, it will make it easier to add changes like #682 in a nicer way, but also should enable better designs in packages such as Revise and JET.