Skip to content

fix(memo): delay implicit output collector#9215

Merged
emillon merged 6 commits intomainfrom
ps/rr/fix_memo___delay_implicit_output_collector
Nov 28, 2023
Merged

fix(memo): delay implicit output collector#9215
emillon merged 6 commits intomainfrom
ps/rr/fix_memo___delay_implicit_output_collector

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg commented Nov 17, 2023

When doing:

let x = Memo.Implicit_output.collect (fun () -> ..) in
let* _ = x in
let* _ = x in

Would share the collecting cell between the binds. That's inconsistent
with delaying the side effects until binds.

Signed-off-by: Rudi Grinberg me@rgrinberg.com

@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Nov 17, 2023

@snowleopard I don't think this affects you, b/c you don't use implicit output, but it's a bit of a footgun.

snowleopard
snowleopard approved these changes Nov 20, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I think internally we do rely on implicit output, so I'd prefer to test/benchmark this change internally first. Please hold on with merging.

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented Nov 20, 2023

Fixes both bugs I observed:

@emillon emillon mentioned this pull request Nov 21, 2023
Closed
26 tasks
@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Nov 21, 2023

Is it possible to add a test for this?

This was linked to issues Nov 21, 2023
multiple rules generated for ccomp #9225
Closed
multiple rules generated for .opam in watch mode #9226
Closed
@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Nov 21, 2023

Added it here #9247

@rgrinberg rgrinberg force-pushed the ps/rr/fix_memo___delay_implicit_output_collector branch from c7d8da6 to 2302d8a Compare November 21, 2023 18:26
@emillon emillon mentioned this pull request Nov 22, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Nov 22, 2023
@emillon
[new release] dune (15 packages) (3.12.0~alpha3)
11fea32 
CHANGES:

- Cherry-pick ocaml/dune#9215 and ocaml/dune#9009 (@emillon)

- dune-build-info: when `version=""` is found in a `META` file, we now return
  `None` as a version string (ocaml/dune#9177, @emillon)
@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Nov 27, 2023

@snowleopard have you been able to test this internally? This is the last issue blocking the release of 3.12.0.

@snowleopard
Copy link
Copy Markdown
Collaborator

snowleopard commented Nov 28, 2023

@snowleopard have you been able to test this internally? This is the last issue blocking the release of 3.12.0.

Started the benchmarks, will report results later today. Sorry for the delay!

snowleopard
snowleopard reviewed Nov 28, 2023
View reviewed changes
@snowleopard
Copy link
Copy Markdown
Collaborator

snowleopard commented Nov 28, 2023

OK, benchmarks look good. I've imported the change as well as the test, and also left a couple of suggestions.

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Nov 28, 2023

I have a quick question. In many places we have let* () = Memo.return () in and I'm never too sure about what this means. Does this change to Memo itself mean that we can remove (some of) these lines?

snowleopard
snowleopard reviewed Nov 28, 2023
View reviewed changes
@snowleopard
Copy link
Copy Markdown
Collaborator

snowleopard commented Nov 28, 2023 •
edited
Loading

I have a quick question. In many places we have let* () = Memo.return () in and I'm never too sure about what this means. Does this change to Memo itself mean that we can remove (some of) these lines?

Memo now has Memo.of_thunk which is a more explicit version of this code snippet. The idea is the same: you want to delay some computation on the right hand side of the bind (sometimes because it does some side-effect but more commonly just to make the computation lazier).

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Nov 28, 2023

Thanks for the explanation.

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Nov 28, 2023

I'll address the review and merge.

rgrinberg and others added 5 commits November 28, 2023 16:59
@rgrinberg @emillon
fix(memo): delay implicit output collector
542d4bb 
When doing:

```
let x = Memo.Implicit_output.collect (fun () -> ..) in
let* _ = x in
let* _ = x in
```

Would share the collecting cell between the binds. That's inconsistent
with delaying the side effects until binds.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: 7829e529-690d-4f62-b046-849783409ee8 -->

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon @snowleopard
Update src/memo/implicit_output.ml
fb8d91f 
Co-authored-by: Andrey Mokhov <amokhov@janestreet.com>
Signed-off-by: Etienne Millon <etienne.millon@gmail.com>
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
use Fiber.of_thunk
c140671 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Update test name
74d8e77 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Update test output
8a06f80 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon force-pushed the ps/rr/fix_memo___delay_implicit_output_collector branch from 1130e58 to 8a06f80 Compare November 28, 2023 16:00
@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Nov 28, 2023

One reason I write let*() = Memo.return () in instead of Memo.of_thunk is because the latter indents the entire of the function to the right. In this case, it doesn't really matter, but sometimes it causes a bunch of ugly wrapping.

@snowleopard
Copy link
Copy Markdown
Collaborator

snowleopard commented Nov 28, 2023

One reason I write let*() = Memo.return () in instead of Memo.of_thunk is because the latter indents the entire of the function to the right. In this case, it doesn't really matter, but sometimes it causes a bunch of ugly wrapping.

Yeah, it's a little annoying but I think I still prefer the explicit version despite the formatting awkwardness.

@emillon emillon merged commit dee6eae into main Nov 28, 2023
@emillon emillon deleted the ps/rr/fix_memo___delay_implicit_output_collector branch November 28, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@snowleopard snowleopard snowleopard approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

multiple rules generated for .opam in watch mode multiple rules generated for ccomp

4 participants

@rgrinberg @Alizter @emillon @snowleopard