Remove procedure cache from the assembler #1411
Conversation
| // This is a phantom procedure - we know its root, but do not have its | ||
| // definition | ||
| break Err(AssemblyError::Failed { |
There was a problem hiding this comment.
It is not really clear to me why we have this error condition here.
There was a problem hiding this comment.
Because the find method expects to resolve a FullyQualifiedProcedureName to a GlobalProcedureIndex or return an error if that isn't possible. This is one of the reasons why it is useful to represent basic information about compiled modules/procedures in the graph, since those procedures will get assigned a GlobalProcedureIndex, and can be used to resolve invocations throughout the compilation graph.
|
|
||
| // We don't have a cache entry yet, but we do want to make sure we don't have a conflicting | ||
| // cache entry with the same MAST root: | ||
| if let Some(cached) = self.find_procedure(&proc_root) { |
There was a problem hiding this comment.
I think we need to discuss adding a new node type to MAST that represents procedure entry, specifically to ensure that the number of procedure locals is represented in the digest. This also lets us recover essential procedure metadata when loading a library from MAST (such as number of procedure locals).
There was a problem hiding this comment.
I think all of these changes should be fine. However, without some tests to verify assembly of various combinations of compiled and source-form modules, it is very difficult to say for sure if there are any issues that fall out of this. Unfortunately, we can't really add all of the ones we need until after we have #1401 merged. We could probably add some tests around phantoms though.
One of the risks of conflating procedures we have information about and those we don't (phantoms), is that there are places where we handled phantoms differently (and now treat them like resolved MAST root), and places where we handled a resolved MAST root differently (and will now treat them like a phantom). This may result in things that would have previously compiled successfully, failing to compile; or vice versa. We'll definitely need a good suite of tests to cover the various ways (successfully and unsuccessfully) that you can assemble a mix of compiled and source-form modules, to ensure that the behavior is correct.
| kind, | ||
| target: target.clone(), | ||
| }); | ||
| } | ||
| Ok(ResolvedTarget::Phantom(_)) => (), |
There was a problem hiding this comment.
We should start populating self.invoked for ResolvedTarget::Phantom, since we use that information to construct the set of procedures referenced transitively from any given procedure in the module being visited. Previously, we only did that for ResolvedTarget::Cached, because we only cared about procedures we actually had on hand, but now that all targets resolved to a MAST root will use ResolvedTarget::Phantom, we should track all such references.
| // This is a phantom procedure - we know its root, but do not have its | ||
| // definition | ||
| break Err(AssemblyError::Failed { |
There was a problem hiding this comment.
Because the find method expects to resolve a FullyQualifiedProcedureName to a GlobalProcedureIndex or return an error if that isn't possible. This is one of the reasons why it is useful to represent basic information about compiled modules/procedures in the graph, since those procedures will get assigned a GlobalProcedureIndex, and can be used to resolve invocations throughout the compilation graph.
Agreed. What I'll do is roll-back the 3rd commit in this PR and then once we have a more complete implementation of different parts, we can re-apply it in a separate PR. |
bobbinth
force-pushed
the
bobbin-remove-proc-cache
branch
from
f71665d
to
6730fc3
Compare
bobbinth
force-pushed
the
bobbin-remove-proc-cache
branch
from
6730fc3
to
d04a0d6
Compare
This PR builds on #1409 and tries to further simplify the assembler by removing
ProcedureCachestruct.The PR consists of 3 commits:
ProcedureCachestruct and moves all relevant functionality into theMastForestBuilderstruct.ResolvedTarget::Cachedvariant. The motivation is that it doesn't really matter if the procedure is cached or not -MastForestBuilderwill make sure that we cannot create nodes with duplicate MAST roots, and so the same procedure cannot be inserted twice into the forest.The last commit completely removes root tracking from theModuleGraph. This means that the name resolver does not try to resolveInvocationTarget::MastRootbut just maps it directly toResolvedTarget::PhantomCall(which I renamed intoResolvedTarget::MastRoot). The idea here is similar to the previous point in thatMastForestBuilderhas enough information to resolve a MAST root to a correct node in the forest.I'm pretty sure that the first commit does not break anything, but I'm less sure about the other two commits (especially the 3rd one). So, it is possible to that I've missed some important details (tests are passing, but it is possible that something is not being tested).
Update: the last commit was removed from this PR as we can't test its effects sufficiently yet.