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

replace private val/var with private[this]. #8199

Closed
wants to merge 1 commit into from

Conversation

@takayahilton
Copy link
Contributor

commented Jul 3, 2019

private [this] val/var is translated to direct field access

@scala-jenkins scala-jenkins added this to the 2.12.9 milestone Jul 3, 2019

@dwijnand

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

What's the motivation behind this change?

It's the more restrictive option which is both good and bad (as things evolve you might have to micromanage this detail). Also it's more verbose. Also this is targeting 2.12.x.

@plokhotnyuk

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

what about private def?

@takayahilton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@dwijnand
private [this] val/var is converted to direct field access, I guess it will perform slightly better than private val/var.

And class file size will also be smaller.
(But I'm not sure)

@takayahilton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@plokhotnyuk
I think there is no difference on byte code between private [this] def and private def.

@plokhotnyuk

This comment has been minimized.

@lrytz

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

I'm not in favor of this change, unless we have some (benchmark) evidence that there's an advantage. From my experience, I would not expect to see any performance benefit (getters/setters are small enough to be inlined unconditionally by the JVM). cc @retronym.

@xuwei-k

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

scala-library.jar size

before: 5187871 byte a995c4d
after: 5178546 byte 98fd6ff
diff: 9325 byte

@lrytz

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

@xuwei-k out of curiosity, how did you build the jars? in particulari, was the optimizer enabled?

@xuwei-k

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

how did you build the jars?

library/packageBin

@retronym

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

Here's the bytecode diff (generated with the help of scafu and jardiff:

$ jardiff $(scala-classpath $(scala-ref-version a995c4d))  $(scala-classpath $(scala-pr-version 8199))| tee /tmp/private-this.diff

https://gist.github.com/retronym/1707779d4428e48c013c33ee89ce04a6

The bytecode size savings are due to avoiding emitting the getter/setter methods. There should be no difference in runtime performance in HotSpot due to the policy to unconditionally inline accessors. Perhaps one would save a few millisecond of startup cost due to less work for JIT to do.

@lrytz

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Thanks. Do you agree to close this one?

@retronym

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Yeah, I think I'd prefer to improve the backend's handling of plain-old-private vals and vars.

It could:

  • emit getfield / setfield (to save the JIT from needing to do the inlining)
  • stop creating the accessors as an opt-in optimization. I don't think we can safely do this by default because some folks might be relying on them via Scala/Java reflection.
@som-snytt

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I hope the language finds its way to make private[this] the default and requiring private[C] for current semantics.

I know what private means in ordinary language. If I have a clone, an identical twin, or a Doppelgänger, I don't want that person reading my diary or rifling through my sock drawer or coming into the bathroom when it's my turn. Frankly, I don't even want my Doppelgänger shadowing me in the streets and showing up at parties unexpectedly.

@lrytz lrytz closed this Jul 17, 2019

@lrytz

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

@takayahilton takayahilton deleted the takayahilton:replace-private-this branch Jul 18, 2019

@SethTisue SethTisue removed this from the 2.12.9 milestone Jul 18, 2019

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