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

@odersky odersky 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 July 30, 2019 16:06
Stale symbol errors can happen when inspecting symbols while comparing
the trees before and after pickling.
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
Restricting `entered` and `enteredAfter` to class members is more a trap to
fall into than a helpful check.
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.
Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

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

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! ☀️

and add pos and neg tests
@jducoeur
Copy link
Contributor

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
Copy link
Contributor Author

odersky 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
Copy link
Member

smarter 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
Copy link
Contributor

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

@odersky
Copy link
Contributor Author

odersky 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants