Do reference-based pruning for ucm compile, turn back on inlining #5507
Conversation
This file contains 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
The pruning was causing problems with compiled programs when inlining was on, because it would prune based on the inlined code. The inlined code may have certain intermediate combinators omitted, but those are still necessary to have a full picture of the source code. Since `compile` was using the MCode numbering and backing out which References are necessary from that, it would throw away the source code for these intermediate definitions. This then caused problems when e.g. cloud (running from a compiled build) would try to send code to other environments. It wouldn't have the intermediate terms necessary for the remote environment to do its own intermediate->interpreter step. This new approach does all the 'necessary terms' tracing at the intermediate level, and then instead determines which MCode level defintions are necessary from that. This means that the pruning is no longer sensitive to the inlining. So, it should be safe to turn inlining back on.
ChrisPenner
approved these changes
Dec 14, 2024
|
Should I go ahead and merge this, or are we waiting for a test? |
|
Thanks! I just kicked off a pre-release and will test this today. But is it possible to add a regression test? Can it somehow be incorporated into the current "golden file" tests or something? Running the klogs demo and seeing if it hangs is a time-consuming way to try to catch this and isn't currently happening in CI. |
|
@ceedubs Probably yes, can we sync up tomorrow or so to figure out a good way to do this |
ceedubs
approved these changes
Dec 16, 2024
aryairani
pushed a commit
that referenced
this pull request
Dec 18, 2024
this specific test may become ineffective if the code serialization algorithm someday includes termlinks as dependencies.
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.
The interpreter state pruning was causing problems with compiled programs when inlining was on, because it would prune based on the inlined code. The inlined code may have certain intermediate combinators omitted, but those are still necessary to have a full picture of the source code. Since
compilewas using theMCodenumbering and backing out whichReferencesare necessary from that, it would throw away the source code for these intermediate definitions. This then caused problems when e.g. cloud (running from a compiled build) would try to send code to other environments. It wouldn't have the intermediate terms necessary for the remote environment to do its ownintermediate->interpreter step.
This new approach does all the 'necessary terms' tracing at the intermediate level, and then instead determines which
MCodelevel defintions are necessary from that. This means that the pruning is no longer sensitive to the inlining. So, it should be safe to turn inlining back on.@ceedubs This fixes my local test, which is just looking up recursive dependencies of something in base in a compiled program (i.e. it works fine doing
runin ucm, because everything is loaded, but not inrun.compiledbecause some of the recursive dependencies have been pruned away). Let me know if it solves the cloud issues.