Make Cthulhu cache CodeInstance-based, continued

[serenity4]

Continues the work started in #614.

Tests are passing locally, this is ready for review.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 245 lines in your changes missing coverage. Please review.

Please upload report for BASE (2.17@ac0a428). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/CthulhuBase.jl	0.00%	72 Missing ⚠️
src/callsite.jl	0.00%	48 Missing ⚠️
src/reflection.jl	0.00%	44 Missing ⚠️
src/codeview.jl	0.00%	38 Missing ⚠️
src/interpreter.jl	0.00%	31 Missing ⚠️
src/interface.jl	0.00%	12 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##             2.17    #628   +/-   ##
======================================
  Coverage        ?   0.00%           
======================================
  Files           ?      11           
  Lines           ?    1642           
  Branches        ?       0           
======================================
  Hits            ?       0           
  Misses          ?    1642           
  Partials        ?       0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[aviatesk]

https://github.com/serenity4/cthulhu.jl/blob/d3ff8213541262ec36f08052b15844a44fefe4b1/src/codeview.jl#L336-L395
Do you think we could refactor this part to use CodeInstance for lookups instead of MethodInstance? It looks like it might let us get rid of get_mi completely.

But this add_callsites! function is only used by an experimental feature in VSCode. So, if it seems too hard or would need a bigger refactoring, it's totally fine to leave it as it is and half-supported.

[serenity4]

Good point, I'll take a look 👍

[aviatesk]

When I handed this PR over to you, I felt that completing it would require a more extensive refactoring than the commits you added (the refactoring to split Cthulhu into the UI part and the AbstractInterpreter integration part, as we discussed before). But I can't remember what piece of code made me feel like that...

That said, looking at this code, it seems like most Cthulhu features are probably supported on this PR, so I think it's okay to proceed as is for now.
We'll probably do that extensive refactoring eventually anyway.

[serenity4]

That may have been EdgeCallInfo previously using a MethodInstance for some of the UI parts, and now having to find a CodeInstance. Fortunately it seems that one could always be obtained in some way from mkinterp at the top-most levels and have it passed around downstream.

[serenity4]

https://github.com/serenity4/cthulhu.jl/blob/d3ff8213541262ec36f08052b15844a44fefe4b1/src/codeview.jl#L336-L395
Do you think we could refactor this part to use CodeInstance for lookups instead of MethodInstance? It looks like it might let us get rid of get_mi completely.

TaskCallInfo (relevant in context of task-spawning callsites) currently wraps an EdgeCallInfo constructed with a MethodInstance, by manually materializing the MethodInstance that corresponds to the start function of the task. It is not trivial to get a CodeInstance for it, because the runtime accepts a generic function argument that will be resolved dynamically. Would it nonetheless be reasonable to run inference on the MethodInstance to get a CodeInstance back? I see that we already run type inference when descending into it, the difference would be that we now do that eagerly when creating the TaskCallInfo.

Other than that, I don't see major obstacles to use CodeInstances for the cache-related parts in add_callsites!. I had a failure for getproperty(::Module, ::Symbol), where unoptimized source does not appear to be available in lookup, which I'll look into. We also still use MethodInstances for a few things (notably, TypedSyntax-related parts), which I am thinking might not gain much by using CodeInstances, so the change does not have to be very intrusive since we can always extract one from the CodeInstance.

[serenity4]

Would it nonetheless be reasonable to run inference on the MethodInstance to get a CodeInstance back?

Implemented in 0ae8e07, now we no longer need to define get_mi(::Callsite).

Tests for the VSCode integration were disabled and I had forgotten to update that part which was still failing. Now it seems to be working alright (at least when I quickly tested it manually), will probably need further testing by uncommenting the relevant testsets but that can be done later.

I had a failure for getproperty(::Module, ::Symbol), where unoptimized source does not appear to be available in lookup

I haven't managed to identify the exact cause beyond the fact that finishinfer! is never called on it, looks like inference does not run for this call; for now I put a temporary hack, we will see if this can happen for other callees. Any insight into what may be happening would be welcome.

Other than that, unless you have other requests before we merge this should be ready.

[aviatesk]

Would it nonetheless be reasonable to run inference on the MethodInstance to get a CodeInstance back?

Yeah, I think it's totally fine to change it that way. Eager inference might add a small performance hit to code with task constructs, but Cthulhu's performance doesn't need to be super optimized anyway. Whatever's easiest to deal with is good.

I haven't managed to identify the exact cause beyond the fact that finishinfer! is never called on it, looks like inference does not run for this call

This issue sounds a bit serious. Does this only happen with inference performed from add_callsites!? I just looked back at the current implementation of abstractinterpretation.jl, and it seems like there is no longer a guarantee that finishinfer! is called for a frame when it is created from const_prop_call.

[serenity4]

My mistake, finishinfer! is actually called; even in const_prop_call, inference goes through typeinf(interp, frame) which ensures that once frames are inferred they go through finishinfer!.

The issue is apparently that, in the cache, we store a result::InferenceResult, and later try to get an InferredSource from this cache using ci::CodeInstance such that ci === result.ci_as_edge. I'll have a deeper look, it seems that the .ci_as_edge field is filled after finishinfer!, so at the time of caching we can't use that.

[serenity4]

I can suggest having a separate data structure in CthulhuInterpreter that stores a list of InferenceResults to be resolved to their ci_as_edge CodeInstances after top-level inference is finished.

[aviatesk]

Does this issue only happen for add_callsites where we need to lookup InferredSource from InferenceKey, or is this a general issue? AFAIU result.ci_as_edge doesn't contain InferredSource, so holding them in the interpreter doesn't allow us to inspect their inferred sources in the Cthulhu cache format.

[serenity4]

Does this issue only happen for add_callsites where we need to lookup InferredSource from InferenceKey, or is this a general issue?

It only happens there as far as I am aware, yes. I use the following reproducer:

using Cthulhu
f() = Base.sum
@descend f() # then descend into `Base.sum` with VSCode integration settings on

AFAIU result.ci_as_edge doesn't contain InferredSource, so holding them in the interpreter doesn't allow us to inspect their inferred sources in the Cthulhu cache format.

I implemented a proof of concept here, this will hopefully better communicate what I meant: 844e6f4

This commit successfully addresses the case above, but I can revert it if you prefer another approach; that was mainly to explore whether that might work in practice.

[aviatesk]

Okay, I understand the situation.

Currently, the Cthulhu cache uses Union{CodeInstance, InferenceResult} as keys. But add_callsites! currently only looks up using CodeInstance (or MethodInstance previously) as the key. Ideally, like in Cthulhu's main driver, we should use InferenceResult as the key for call sites that have been const-prop'ed. However, the function haven't been implemented in that way, so I'm not sure if it's even possible.

A more general refactoring idea would be to stop using InferenceResult in the Julia compiler and manage everything associated with CodeInstance. That way, Cthulhu's cache could also be simplified to use CodeInstance as the only key type.

However, this feature isn't Cthulhu's main focus, and it's experimental anyway, so I'll leave it up to you to decide how to proceed.

[serenity4]

Thanks for the clarifications. Thinking about it, I don't really like the idea of adding a field to CthulhuInterpreter for the purpose of patching this very issue, and at the same time it appears to me that exploring const-proped callsites is not particularly useful for add_callsites!. I reverted the previous commit and went with 8d2b4fb which should fix the issue in a minimally intrusive way. We can proceed with this fix if you think that it is reasonable.

[aviatesk]

I merged #619, and also updated the 2.17 branch. I'm afraid but could you please rebase this branch? We can go ahead and merge this and release 2.17 then.

[aviatesk]

We can proceed with this fix if you think that it is reasonable.

Yeah, that also sounds reasonable. Let's move ahead with it.

[serenity4]

I merged #619, and also updated the 2.17 branch. I'm afraid but could you please rebase this branch? We can go ahead and merge this and release 2.17 then.

Do you mean release 2.17 for 1.12 only? I thought this PR would have to drop support for 1.10/1.11.

[aviatesk]

Yeah, that's the plan.

In v1.11, the inference cache uses MethodInstance as the key, so we can't do this refactoring.

Starting with v1.12, we want to move forward with development based on this refactoring.

After v1.12 is officially released, we'll stop supporting v1.11. The we'll merge the v2.17 branch into master and continue development as usual.

[serenity4]

Sounds good! I updated the project and CI versions to use 1.12+ (for Cthulhu only).

[serenity4]

After this is merged to 2.17 I'll make another PR to get rid of the outdated version checks to clean up the code a bit.

[aviatesk]

Thanks for moving this PR forward!

[aviatesk]

After this is merged to 2.17 I'll make another PR to get rid of the outdated version checks to clean up the code a bit.

That's great. Please carry on.