Skip to content

Use a final module instance field, assigned in <clinit> where possible #6523

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.

Sign up for GitHub

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

Jump to bottom
Merged
lrytz merged 1 commit into from
May 25, 2018
Merged

Use a final module instance field, assigned in <clinit> where possible #6523

lrytz merged 1 commit into from
May 25, 2018

Conversation

@retronym
Copy link
Member

@retronym retronym commented Apr 14, 2018 •
edited
Loading

For modules without side effects in their constructors, and that only extend Object or (Abstract)FunctionN, we can defer assignment of the module instance field, and make that field final.

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Apr 14, 2018
@retronym retronym added the performance the need for speed. usually compiler performance, sometimes runtime performance. label Apr 14, 2018
@retronym retronym mentioned this pull request Apr 14, 2018
Open
@retronym retronym added the WIP label Apr 14, 2018
@mkeskells
Copy link
Contributor

mkeskells commented Apr 17, 2018

Implementation could be widened to cover Predef
if the parent class constructor (and all transitive parents) don't access this optimisation is safe.
Separate compilation is an issue, but if all of the parents are in the same compilation unit it should be OK as I see it, and that is true for Predef

In general it would be nice to be able to mark pure behaviours (that don't access this) in some way to enforce the limit at compile time. or even make these implementatation methods static? (this that is not too offensive to scala devs 🔥) - rorygraves#19

@adriaanm adriaanm modified the milestones: 2.13.0-M4, 2.13.0-M5 Apr 30, 2018
@retronym retronym force-pushed the topic/static-module branch from b23cea3 to b7887bb Compare May 3, 2018 23:34
@retronym
Copy link
Member Author

retronym commented May 3, 2018

@mkeskells The constructor matters, not just the parents, and Predef forces initialiazation of scala.package, s.c.i.List, and a few others. The initializers of those expect that Predef.MODULE$ is non-null.

@retronym retronym force-pushed the topic/static-module branch from b7887bb to 85c1764 Compare May 4, 2018 00:12
@retronym retronym changed the title WIP Use a final module instance field, assigned in <clinit> where possible Use a final module instance field, assigned in <clinit> where possible May 4, 2018
@retronym retronym removed the WIP label May 4, 2018
@retronym retronym mentioned this pull request May 13, 2018
Closed
@retronym retronym requested a review from lrytz May 24, 2018 00:30
lrytz
lrytz reviewed May 24, 2018
View reviewed changes
Copy link
Member

@lrytz lrytz 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 otherwise. Maybe add a test/junit/scala/lang/modules/BytecodeTest.scala?

src/library/scala/Predef.scala Outdated
object Predef extends LowPriorityImplicits {
def main(args: Array[String]): Unit = {

}
Copy link
Member

@lrytz lrytz May 24, 2018

Choose a reason for hiding this comment

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

There's some leftover here

Copy link
Member Author

@retronym retronym May 24, 2018

Choose a reason for hiding this comment

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

oops!

@retronym retronym self-assigned this May 24, 2018
@retronym
Use a final module instance field, assigned in <clinit> where possible
443e175 
For modules without side effects in their constructors, and that
only extend Object of FunctionN, we can defer assignment of the module
instance field, and make that field final.
@retronym retronym force-pushed the topic/static-module branch from 85c1764 to 443e175 Compare May 25, 2018 01:40
@retronym
Copy link
Member Author

retronym commented May 25, 2018 •
edited
Loading

I've added a test which showed up a problem in the implementation (pure case class companions extends AbstractFunctionN and Serializable)

@retronym
Copy link
Member Author

retronym commented May 25, 2018 •
edited
Loading

A more general approach is proposed in forax/exotic, which uses invokedynamic to create a ConstantCallSite once the non-null value has been observed. This is a user-level version of @java.lang.invoke.Stable, which is currently JDK-internal.

@mkeskells
Copy link
Contributor

mkeskells commented May 25, 2018

@retronym did you confirm if that would well on Java 8. The exotics library doesn't compile in Java 8, and some of the techniques that I have back-ported are slow until Java 9. I have not catalogued which ones though

@lrytz lrytz merged commit 4089618 into scala:2.13.x May 25, 2018
@retronym
Copy link
Member Author

retronym commented May 25, 2018 •
edited
Loading

I'm not planning to pursue the invokedynamic approach at the moment, but when we do, you're absolutely right in noting that we need to test the range of JDKs we support and budget time for surprises!

@mkeskells mkeskells mentioned this pull request Jul 5, 2018
Open
@retronym retronym mentioned this pull request Aug 14, 2018
Closed
@retronym retronym mentioned this pull request Sep 11, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lrytz lrytz lrytz approved these changes

Assignees

@retronym retronym

Labels

performance the need for speed. usually compiler performance, sometimes runtime performance.

Projects

None yet

Milestone

2.13.0-M5

Development

Successfully merging this pull request may close these issues.

5 participants

@retronym @mkeskells @lrytz @adriaanm @scala-jenkins