Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delay applyExpr until after elaboration #732

Conversation

vrindisbacher
Copy link
Collaborator

@vrindisbacher vrindisbacher commented Jan 17, 2025

There are a few changes I'm hoping to implement here. There are:

  1. delaying use of applyExpr in elab until after all the elaboration has taken place.
  2. delaying use of apply in elab until after all the elaboration has taken place - hoping to use union find to do this. See the PoC implementation here.
  3. Separate type visitor and type folder into two separate structures. Additionally, ditch using the State monad for folding and replace it with the Reader monad - similar to what is implemented for CheckM

The reasons for these changes are performance hang ups when passing large queries to fixpoint. We noticed that a significant amount of time is being spent in elab - and we suspect it's due to the exponential nature of using apply and applyExpr. Additionally, we observed that trans and fold in the type visitors and folders were taking up a massive amount of memory. By "specializing" the visitor not to have an accumulator we reduced the memory footprint on certain queries by ~ half.

As of now, changes 1 and 3 are implemented and I'm working on 2. Just 1 and 3 cut down the time fixpoint takes on large queries by ~ 1/3.

@ranjitjhala
Copy link
Member

let me check this works with the LH tests too...

@vrindisbacher
Copy link
Collaborator Author

Sounds good - fwiw I also ran the flux tests with this version and everything passed.

@nilehmann
Copy link
Contributor

nilehmann commented Jan 17, 2025

I'm trying to rationalize the correctness of this change.

I think you can delay the substitution of sort variables as long as you don't structurally match on sorts, i.e., you don't make decisions based on the structure of sorts (which may have solved variables that should be considered in the decision). Presumably, elaboration shouldn't be making decisions based on the structure of sorts in the expression being elaborated so this change sounds fine...

...except that you structurally match on sorts when printing expressions for errors. For example, I think this changes the behavior of this line

!s <- checkIteTy g p e1' e2' s1 s2
because checkIteTy prints the expressions (with pending substitutions) in case of an error. However, I have the impression that this always has been wrong and errors may print expressions with sorts variables that have already been solved.

unrelated and out of curiosity: do the changes in strictness have any impact? the constructors of Expr are already marked as strict

@vrindisbacher
Copy link
Collaborator Author

Ah ya you're right - I suppose you could use applyExpr in the error case of checkIteTy. e..g errIte e1 e2 t1 t2 -> errIte (applyExp theta e1) (applyExp theta e2) t1 t2?

@vrindisbacher
Copy link
Collaborator Author

vrindisbacher commented Jan 19, 2025

I think there are some bugs in Foldable. Working them out now.

@vrindisbacher vrindisbacher force-pushed the vrindisbacher/delay-applyExpr branch from 8fea3d5 to 1a12686 Compare January 20, 2025 18:38
@facundominguez
Copy link
Collaborator

Hello! Is there a description available of the problem that this contribution is addressing? I don't promise to review, but it would be helpful for future reference nonetheless.

@vrindisbacher vrindisbacher force-pushed the vrindisbacher/delay-applyExpr branch from 6710ee0 to 140ae73 Compare January 24, 2025 15:01
@vrindisbacher
Copy link
Collaborator Author

Hello! Is there a description available of the problem that this contribution is addressing? I don't promise to review, but it would be helpful for future reference nonetheless.

I just updated the description with some details. Let me know if you have questions / if I can provide additional details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants