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

feat: generalize ReaderT instances #1447

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

digama0
Copy link
Collaborator

@digama0 digama0 commented Aug 7, 2022

There are no behavioral changes here, only generalizing things like "ReaderT r m is an applicative when m is" to all the defined semimonad types instead of assuming Monad for everything.

@digama0 digama0 force-pushed the readert_inst branch 4 times, most recently from 8ca6fe4 to c60f108 Compare August 9, 2022 16:58
@leodemoura
Copy link
Member

bench!

@digama0
Copy link
Collaborator Author

digama0 commented Aug 10, 2022

I think the bang is on the other side. Can I do that?

!bench

...guess not.

@leodemoura
Copy link
Member

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit c60f108.
There were significant changes against commit 578adcd:

  Benchmark   Metric       Change
  ========================================
- stdlib      task-clock       1% (77.2 σ)

@leodemoura
Copy link
Member

!bench

@leodemoura
Copy link
Member

@digama0 Did you find out what was triggering the stack overflow in debug mode?

@digama0
Copy link
Collaborator Author

digama0 commented Aug 11, 2022

No, in the end I ran out of time / patience on debugging and just did black box testing to find the part of the PR that doesn't hit the error. That's also why it scaled back from the original intent (it's not really generalizing the instances anymore, only adding implementations for the missing functions). If we could find the culprit we might be able to do this better later.

If you are interested in pursuing this, here's a list of things that cause the error:

  • Inlining ReaderT.pure or ReaderT.bind into the instance
  • Generalizing any of the instances to, e.g. [Pure m] : Pure (ReaderT r m) instead of [Monad m] : Pure (ReaderT r m)
    • This includes ReaderT.pure, even though it's not even an instance

The working theory is that inlining these cause lean to be more aggressive about inlining generally for monad stacks including ReaderT. We generally want ReaderT to be inlined, but it is possible that there is something else that we don't want to inline that is being hidden behind the ReaderT implementation. The error occurred in debug mode when elaborating 1365.lean, so it is likely during the construction of the Repr instance, but concretely it is a stack overflow that occurs in instantiateMVars inside mkInjectiveTheorems.

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit ebf5b1a.
There were no significant changes.

@leodemoura leodemoura merged commit 6906a4d into leanprover:master Aug 12, 2022
@leodemoura
Copy link
Member

@digama0 Thanks for the explanation. I will try to take a look later at the StackOverflow issue.

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.

None yet

3 participants