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

[ performance ] Implement weak memoisation of lazy values for chez and racket #2791

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

buzden
Copy link
Contributor

@buzden buzden commented Dec 5, 2022

This PR implements a weak memoisation of lazy values for the chez backend as an codegen option (directive). Weakness is in the fact that no guarantees of no re-evaluation of a lazy value is given, and those memoised values can be garbage collected at any wish of the GC. I think, this is reasonable middling between current full re-evaluation and full memoisation behaviours, not requiring addition of separate type-constructor-like construction along with Lazy and Inf, as it was discussed in #1013.

Behaviour of other backends than chez and racket should be unaffected by this change.

@gallais gallais added the event: IDM 2022/12 Issue tackled during the December 2022 Idris Developers Meeting label Dec 6, 2022
@buzden buzden changed the title [ lazy ] Implement weak memoisation of lazy values for chez [ performance ] Implement weak memoisation of lazy values for chez Dec 12, 2022
@buzden buzden changed the title [ performance ] Implement weak memoisation of lazy values for chez [ performance ] Implement weak memoisation of lazy values for chez and racket Dec 13, 2022
@buzden buzden force-pushed the memo-lazy branch 2 times, most recently from bbb3ac4 to 2f18703 Compare December 14, 2022 14:34
@buzden buzden force-pushed the memo-lazy branch 3 times, most recently from 6cc2c30 to 726a419 Compare December 21, 2022 09:14
@buzden buzden force-pushed the memo-lazy branch 2 times, most recently from 2889378 to 5429cc1 Compare December 26, 2022 21:42
@buzden buzden force-pushed the memo-lazy branch 2 times, most recently from 9860fd4 to 48a0520 Compare January 25, 2023 11:19
@buzden buzden force-pushed the memo-lazy branch 2 times, most recently from eb414af to 1659dea Compare February 6, 2023 06:35
@gallais gallais self-assigned this Feb 8, 2023
@gallais
Copy link
Member

gallais commented Feb 8, 2023

Testing this by running rm -r build/ && time idris2 --build idris2api.ipkg
on this branch (with main merged in) vs main and I get a slight slowdown (~5%):

main:
  2m46s (real 2m52s)
  2m47s (real 2m54s)
  2m46s (real 2m53s)

branch:
  2m55s (real 3m02s)
  2m56s (real 3m02s)
  2m55s (real 3m02s)

@gallais gallais removed their assignment Feb 8, 2023
@buzden
Copy link
Contributor Author

buzden commented Feb 8, 2023

I didn't repeat my tests for a while, so I may guess that the bigger slowdown is induced by the last commit, when additional boxing is used.

Note around this commit. We've been using this PR's branch for a while in running algorithm that heavily relies on memoisation and realised very strange and flacky behaviour of chez complaining on invalid memory references (each time in different places and even not related to lazy values; looks like normal objects got freed accidentally). We've found out that additional boxing solves this (i.e. this does not happen when weak reference to memoised value is never a primitive value, always a box), which looks like a kind of a bug in GC of chez. However, I didn't manage to reproduce this behaviour with a small example, thus I couldn't add a test for it nor report it to chez.

Anyway, this boxing led to a visible slowdown in my "worst-case-ish" example, which I talked about at IDM or on Discord. But at the time, at least, together with independent improvements in performance since then, the total performance was no worse than the very original, when this change started to be developed.

So, it's hard to decide whether is it okay to pay such a slowdown price for an ability to gain performance in memoisation-relying algorithms. I'm definitely biased, since I use one of such algorithms, so I think it's worth doing.

@buzden
Copy link
Contributor Author

buzden commented Jun 5, 2024

Okay, I finally made proposed change to be guarded by a codegen directive, and to be turned off by default.

So, there is no change in runtime penalty when codegen directive is off. But now users are able to turn this on, either by --directive or using %cg pragma (you can see both in the tests). Newly added tests which contain this pragma pass only with this option turned on.

I hope, this would this change to be finally considered to be included, as it does not affect runtime performance of existing code.

I squashed my previous commits, so that now PR contains two: one for implementation of weak memoisation itself, and the second for making it a codegen directive, I hope it would help the review.

I added additional runs with the directive turned on to the */futures* tests just in case to test feature interference like once was found with #3292.

I made chez's incremental compilation output to ignore this directive, because it is not safe to take it into account since all different parts can go out of sync, which would lead to runtime fails.

@gallais gallais merged commit 2482ebb into idris-lang:main Aug 6, 2024
22 checks passed
@buzden buzden deleted the memo-lazy branch August 6, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event: IDM 2022/12 Issue tackled during the December 2022 Idris Developers Meeting performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants