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

[ fix #2927 ] Do not eagerly evaluate expressions occurring inside %delay block #2939

Merged
merged 4 commits into from
May 31, 2024

Conversation

AlgebraicWolf
Copy link
Contributor

@AlgebraicWolf AlgebraicWolf commented Apr 4, 2023

Description

This PR fixes #2927.

Thanks to @stefan-hoeck for his assistance in Discord. He suggested fixing the issue by using Scheme's delay and force to memoize constants, as in #2807 instead of #2817 that is currently used. However, this solution comes with a performance overhead.

To reduce the overhead, I modified CSE to track whether an expression occurs inside a %delay block. If that is the case, the expression is lifted as memoized lazy value using Scheme's delay and force. Otherwise, it becomes an eagerly evaluated top-level constant. Benchmarking the bootstrapped compiler on idris2api.ipkg showed 5% slowdown on my machine, which is an improvement compared to the 8.5% slowdown in #2807.

Should this change go in the CHANGELOG?

  • If this is a fix, user-facing change, a compiler change, or a new paper
    implementation, I have updated CHANGELOG.md (and potentially also
    CONTRIBUTORS.md).

@AlgebraicWolf AlgebraicWolf changed the title [ fix #2928 ] Do not eagerly evaluate expressions occurring inside %delay block [ fix #2927 ] Do not eagerly evaluate expressions occurring inside %delay block Apr 4, 2023
@CodingCellist
Copy link
Collaborator

Is this related to #2791 ? : )

@AlgebraicWolf
Copy link
Contributor Author

Is this related to #2791 ? : )

No, not really. Moreover, this PR uses proper memoization instead of relying on #2791, as it yields better performance in this specific case

I guess the only way they are related is that both the need for #2791 and discovery of #2927 happened because of an attempt to manipulate large lazily-constructed data structures

@gallais
Copy link
Member

gallais commented Jul 31, 2023

This makes sense to me however it would be nice to know what the cause
of the slowdown is. I don't think we're using many top level lazy constants
in the compiler.

@mjustus
Copy link
Collaborator

mjustus commented May 28, 2024

Seeing that the current CSE is unsound with respect to %delay, I think this should be merged unless our backend maintainers (@stefan-hoeck, in particular) have concerns.

@stefan-hoeck
Copy link
Contributor

Seeing that the current CSE is unsound with respect to %delay, I think this should be merged unless our backend maintainers (@stefan-hoeck, in particular) have concerns.

To the best of my knowledge, we have (a) test(s) to verify that non-delayed top-level constants get converted to constants correctly. If these tests still pass (they do), I'm all for merging this.

@AlgebraicWolf
Copy link
Contributor Author

I have rebased this PR, all the tests still do pass. Perhaps it's worth re-measuring compilation time as well

@mjustus
Copy link
Collaborator

mjustus commented May 31, 2024

Thanks @stefan-hoeck! I am merging this now and am happy to take the blame if something goes wrong.

@mjustus mjustus merged commit 10b0cc3 into idris-lang:main May 31, 2024
22 checks passed
@andrevidela
Copy link
Collaborator

Software's not about blame! Don't feel pressure to mess up something.

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

Successfully merging this pull request may close these issues.

CSE optimisation is tough with %inline and lazy meeting in the same place
6 participants