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

Alternative fix for #6909: Use @cached annotation #6967

Closed
wants to merge 8 commits into from

Conversation

@odersky
Copy link
Contributor

commented Jul 30, 2019

Instead of memo, have a @cached annotation. Usage example:

@scala.annotation.meta.cached def foo = rhs

A @cached def is like a lazy val, except that

  • it's not synchronized
  • it uses null as uninitialized value
  • it can have given parameters

Use the same logic for caching given aliases

odersky added 7 commits Jul 18, 2019
Don't exit with StaleSymbol errors when testing pickling.
Stale symbol errors can happen when inspecting symbols while comparing
the trees before and after pickling.
Add @cached annotation
Syntax:

```
@scala.annotation.meta.cached def foo = rhs
```
It's like a lazy val, except that

 - it's not synchronized
 - it uses `null` as uninitialized value
 - it can have given parameters

Use the same logic for caching given aliases
Make entered and enteredAfter also work for term members
Restricting `entered` and `enteredAfter` to class members is more a trap to
fall into than a helpful check.
Update docs
The new implementation does not optimize given aliases with pure paths as right hand sides to be
vals anymore. The reason is that even a pure path might be expensive to compute so we might want
the caching. And caching is cheap, so there's little downside. Therefore, it's attractive to go
with the simpler two way choice: leave RHS as is, or wrap it in a memo.
@dotty-bot
Copy link

left a comment

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

  1. Separate subject from body with a blank line
  2. When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:
  3. Limit the subject line to 72 characters
  4. Capitalize the subject line
  5. Do not end the subject line with a period
  6. Use the imperative mood in the subject line ("Add" instead of "Added")
  7. Wrap the body at 80 characters
  8. Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

Check @cached annotation for applicability
and add pos and neg tests
@jducoeur

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Out of curiosity, would this be available to userland code? I can definitely see it being useful, at least for folks who know what they're doing...

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@jducoeur That's to be discussed. Im this PR @cached is available as an annotation in package scala.annotation.meta.

@smarter

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

It seems that this is very close to @threadUnsafe lazy val, can we remove one of those in favor of the other ?

@propensive

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Maybe I'm not understanding the discriminating factors, but I think this feature deserves a first-class keyword rather than an annotation.

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

In light of the progress made with the lazy vals design in #6979, it seems safer and more systematic to just use a lazy val for caching given instances.

That leaves the question whether @cached in its present form should be made available as a macro. I am not sure about that anymore. It does not seem to belong in the language spec. The compiler will have some usage for @cached but it's not clear it's enough to warrant the expense of having to implement it.

@odersky odersky closed this Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.