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

Suspended atom fields are evaluated only once #6151

Merged
merged 20 commits into from
Apr 5, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Mar 30, 2023

Pull Request Description

Implements #6134.

Important Notes

One can define lazy atom fields as:

type Lazy
    Value ~x ~y

the evaluation of the x and y fields is then delayed until they are needed. The evaluation happens once. Then the computed value is kept in the atom for further use.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • All code follows the
    Java,
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach linked an issue Mar 30, 2023 that may be closed by this pull request
4 tasks
@radeusgd
Copy link
Member

radeusgd commented Apr 4, 2023

Shall we now remove the Lazy.enso from stdlib and replace its usages with ~ fields?
I'd also keep the Lazy_Spec (only remove the eager part, it's worth keeping the rest as the tests nicely serve as additional documentation of this capability) - we can make it still work with:

type Lazy
    Value ~get
    new ~computation = Lazy.Value computation

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Apr 5, 2023

Shall we now remove the Lazy.enso from stdlib and replace its usages with ~ fields?

That's the overall goal. I found just a single usage in SQL_Type_Reference and removed it in 39819fa

I'd also keep the Lazy_Spec

Good idea. Implemented in 160068a

(only remove the eager part,

Eager part works as well.

it's worth keeping the rest as the tests nicely serve as additional documentation of this capability

There was one failing test before cad4e8e was integrated:

enso$ ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/bin/enso --run test/Tests/src/Runtime/Lazy_Spec.enso
Lazy:  [5/6, 162ms]
    - should compute the result only once [21ms]
    - should compute the result only once, even if copied [10ms]
    - should cache the result even if it results in a dataflow error [28ms]
    - [FAILED] should cache the result even if the operation panics [81ms]
        Reason: 2 did not equal 1 (at /home/devel/NetBeansProjects/enso/enso/test/Tests/src/Runtime/Lazy_Spec.enso:72:9-32).
    - should allow to create a Lazy instance that is computed eagerly [8ms]
    - eager mode will not handle dataflow errors/panics specially [11ms]
5 tests succeeded.
1 tests failed.
0 tests skipped.

I'll check what it would take to implement the panic support. Done in cad4e8e

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Apr 5, 2023

bcec688 expands ListBenchmarks with a Lenivý lazy list variant. The lazy list is approximately 10-20% slower - which seems good enough, given the additional check it has to do to remove laziness:

Result "org.enso.interpreter.bench.benchmarks.semantic.ListBenchmarks.mapOverLazyList":
Iteration : 115.037 ms/op

Result "org.enso.interpreter.bench.benchmarks.semantic.ListBenchmarks.mapOverList":
Iteration: 97.133 ms/op

Those 10-20% are only being paid if one requests the laziness. Existing (strict) benchmarks are unaffected by this change. Running benchmarks to verify that - at first sight the results look OK. Let's integrate!

Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

Nice abuse of the layouting system! Didn't even think this would become possible when I designed it, good to know it extends.

@radeusgd
Copy link
Member

radeusgd commented Apr 5, 2023

This looks great 🎉

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Apr 5, 2023
@JaroslavTulach JaroslavTulach requested a review from Akirathan April 5, 2023 14:23
@mergify mergify bot merged commit 741b394 into develop Apr 5, 2023
@mergify mergify bot deleted the wip/jtulach/LazyAtomFields_6134 branch April 5, 2023 23:46
Procrat added a commit that referenced this pull request Apr 6, 2023
* develop:
  Project Sharing (#6077)
  Adjust `{Table|Column}.parse` to use `Value_Type` (#6213)
  Add cloud endpoints for frontend (#6002)
  Implement `Table.union` for Database backend (#6204)
  Batch insert suggestions (#6189)
  Formatter fix to not fail when encountering an invalid symlink. (#6172)
  Suspended atom fields are evaluated only once (#6151)
  Text.to_display_text is (shortened) identity (#6174)
  Engine benchmark visualization tool can compare two bench runs (#6198)
  Add PRIVATE so function hidden from Component Browser and other tidying... (#6207)
  Hotfix for #6203. (#6210)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Easy way to delay & cache computed atom fields
6 participants