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

Re-implement lazy vals. #7140

Closed
odersky opened this issue Aug 30, 2019 · 20 comments
Closed

Re-implement lazy vals. #7140

odersky opened this issue Aug 30, 2019 · 20 comments
Assignees
Labels
Semester Project Good project to be done by an MSc or strong Bsc computer science student in one semester stat:taken This project is already worked on by a student or as part of another volunteership program

Comments

@odersky
Copy link
Contributor

odersky commented Aug 30, 2019

@liufengyun
Copy link
Contributor

Once this is implemented, we should address one TODO in the following PR:

https://github.com/lampepfl/dotty/pull/9181/files#diff-8dc5ee2dbf11efaad4326519edfa4e45R132

The problem is that the current encoding assumes bitmap fields are non-static, thus in #9181 we cannot mark all fields of module classes as static.

@olsdavis
Copy link
Contributor

Hi, I'm currently working on this, assuming the latest scheme is in this file, in the heading comment. Implementing it, I have realized I would need to sort a few things out (I will be referring to lines in the code I linked):

  • in case Evaluating, why is the CAS performed on x instead of _x? I guess it's a small typo;
  • in case current: Waiting, why is _x assigned to current.awaitRelease(), given that the latter returns Unit? I believe this is a trace of the first version of the new scheme;
  • in case null, how to assign the special value NULL to result, given it is of type A? I have assumed that Evaluating and NULL could be represented as runtime objects, leveraging the AnyRef type of _x like the class Waiting does; but maybe there is a better way to represent these states. If using objects isn't an issue, then maybe we could/should change result to an AnyRef.

@olsdavis
Copy link
Contributor

Hi, I've also noticed that PatternMatcher comes before LazyVals, so I guess I will have to express it directly with if s.

@nicolasstucki nicolasstucki modified the milestones: 3.1.0, 3.2.0 Aug 24, 2021
@olsdavis
Copy link
Contributor

I'm having some trouble with the following:

Unsafe's compareAndSwapObject seems to be much more adapted for object fields: its first argument must be the instance containing the field to perform the CAS on. It seems that passing the class object (i.e. TheClass.class in Java) as a first argument works in order to update a static field (that isn't of a primitive type); however, this answer suggests that an extra Object obtained by calling staticFieldBase should be used to safely access a static field when using CAS. (It also points out the issue that some Unsafe methods have been replaced since Java 9, which creates another problem for maintainability.)

Therefore, should an extra field be created to store this extra Object for static lazy vals? Or maybe a static field can be avoided in the first place?

@odersky odersky moved this from To do to In progress in Scala 3.x planning Sep 9, 2021
@odersky
Copy link
Contributor Author

odersky commented Oct 4, 2021

You mean for local lazy vals? I think we might need to box them, which means we have an enclosing object for them. Note that the current lazyvals implementation does the same thing.

@anatoliykmetyuk anatoliykmetyuk modified the milestones: 3.2.0, 3.3.0 Jan 10, 2022
@anatoliykmetyuk anatoliykmetyuk added the Semester Project Good project to be done by an MSc or strong Bsc computer science student in one semester label Jan 10, 2022
@anatoliykmetyuk
Copy link
Contributor

@olsdavis I remember you were saying you may have some time this year to work on this - just checking with you quickly to see if that's the case?

@olsdavis
Copy link
Contributor

Hi, @anatoliykmetyuk. I'm back on it.

The only issue I'm having is that I want to write this Java expression in Scala: Class<?> c = CurrentClass.class;. I tried using classOf[CurrentClass] from Predef, but when I try invoking it, it claims that the function takes no type parameter. I guess it is because type parameters get erased at a previous stage. So how can I refer to the class object of a symbol?

Why I am asking that, it is because the first argument of the CAS is the instance "over which" the CAS should be done. In static context, giving the class instead of an instance works.

Therefore, I also need to know how to determine whether a field is going to be "static". (So I can know what to CAS over.)

@smarter
Copy link
Member

smarter commented Jan 11, 2022

Instead of generating a classOf tree you can use tpd.clsOf which will generate the appropriate tree for the current compiler phase.

@anatoliykmetyuk anatoliykmetyuk added the stat:taken This project is already worked on by a student or as part of another volunteership program label Jan 24, 2022
@olsdavis
Copy link
Contributor

olsdavis commented Feb 3, 2022

All Tasty tests seem to pass. However, some CompilationTests fail. Namely, i1692.

I must say that I do not understand why it expects the results it requires. Does anyone have more knowledge about this specific test?

Basically, there is the following class:

class LazyNullable(a: => Int) {
  lazy val l0 = a // null out a
  // ...
}

It is instantiated with 'A'.toInt and the test expects that 1. l0 == 'A'.toInt (reasonable), 2. a is null. The second assertion fails with my code, but why would a be null anyway? Is it some odd semantic of lazy vals? If so, is it needed?

@smarter
Copy link
Member

smarter commented Feb 3, 2022

Yes, this is needed to avoid memory leaks, it's only done if a is private and the compiler is sure that it isn't used anywhere else, see the pr where this was impleemnted in dotty (scala 2 did the same before): #4289

@sjrd
Copy link
Member

sjrd commented Feb 3, 2022

This is needed for memory management. We should release references to a as soon as possible so that any memory that a holds on to can be reclaimed by the GC. To do that, there is an overall guarantee that:

  • If there is a private[this] field f, and
  • f is only read in the body of a (unique) lazy val x,
  • then after x is computed, f is zeroed out (nulled out)

@olsdavis
Copy link
Contributor

olsdavis commented Feb 7, 2022

OK, thanks a lot. I think I am almost done.

@olsdavis
Copy link
Contributor

Hi, so, when I last wrote I just had a few small issues with the tests, as almost all pass except for the following:

  • DottyBytecodeTests.scala verifies whether the size of the generated bytecode is equal to some value, but I have changed the way the offset is stored to just a simple long variable. Should I re-apply the same "compression" that was applied to fix this? Even if I do so, I don't believe that the bytecode will be equal in size anyway, or should it? If not, then probably the test should be changed, I guess;
  • The test 7723_1.scala fails because of a name conflict 7721_1.scala, in tests/run/alpha-modules-2. I don't believe it has anything to do with my code, but why is it so? Is it probably just a local installation issue?

Thanks in advance!

@odersky
Copy link
Contributor Author

odersky commented Feb 20, 2022

I am not sure about the DottyBytecodeTests issue. What test fails, and what compression are we talking about?

The 7723_1.scala failure is probably a local installation issue. You can try deleting all class files in your output directory, that is likely to fix the problem.

@olsdavis
Copy link
Contributor

A clean build did fix 7723_1.scala, thank you.

Sorry for the lack of precision: in DottyBytecodeTests, the test lazyFields verifies that the number of instructions in the end bytecode generated by the thread-safe lazy val is equal to 94. I believe this constant should be updated, shouldn't it? (The new answer would be 126.)

I am glad to announce that the latter is actually the only test not to pass!

@odersky
Copy link
Contributor Author

odersky commented Feb 21, 2022

Sorry for the lack of precision: in DottyBytecodeTests, the test lazyFields verifies that the number of instructions in the end bytecode generated by the thread-safe lazy val is equal to 94. I believe this constant should be updated, shouldn't it? (The new answer would be 126.)

Yes, a difference would be expected here. The only question to ask is whether we are comfortable with the increase in code size. But that's best done by a discussion on a PR.

Congrats and thank you for getting this far! Looking forward to see the PR.

olsdavis added a commit to olsdavis/dotty that referenced this issue Feb 23, 2022
Fix/Fixing/Fixes/Close/Closing/Refs scala#7140
olsdavis added a commit to olsdavis/dotty that referenced this issue Feb 23, 2022
- Implements a propotype of the new lazy vals scheme
Fix/Fixing/Fixes/Close/Closing/Refs scala#7140
olsdavis added a commit to olsdavis/dotty that referenced this issue Apr 4, 2022
- Implements a propotype of the new lazy vals scheme
Fix/Fixing/Fixes/Close/Closing/Refs scala#7140
@SethTisue
Copy link
Member

superseded by #15296

@dwijnand
Copy link
Member

This is the issue that #15296 fixes.

@dwijnand dwijnand reopened this Jul 26, 2022
@SethTisue
Copy link
Member

I'm sorry, I don't know how I confused that.

@som-snytt
Copy link
Contributor

I'll take this opportunity to say that I totally get the confusion. I often have to pause and ask is this the issue or the PR? and I've seen others mix them up when commenting, as well.

Sorry for the O/T observation that github needs a threading model.

I want "one-stop shopping" for the comments and abandoned PRs for a topic.

To complement the threading model, I want a "needle" feature that will filter for just the current PR.

szymon-rd added a commit that referenced this issue Oct 28, 2022
Cleaned up version of #14545

Implementing the new lazy vals scheme mentioned in
#6979, fixing
#7140

New PR to avoid force pushing to main branch of the previous author.

Fixes #15149 as well
Applied #14780
@Kordyjan Kordyjan removed this from the 3.3.0-RC1 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Semester Project Good project to be done by an MSc or strong Bsc computer science student in one semester stat:taken This project is already worked on by a student or as part of another volunteership program
Projects
No open projects
Scala 3.x planning
  
In progress
Development

No branches or pull requests