Usually inlined, not always. From the infamous Coda Hale rant:
4. Always use private[this]. Doing so avoids turning simple field access into an
> invokevirtual on generated getters and setters. Generally HotSpot would end up
> inlining these, but inside our inner serialization loop this made a huge
> difference.
>
>
On Wed, Feb 12, 2014 at 2:44 PM, Aaron Davidson <ilike...@gmail.com> wrote:
> I believe the performance implications are negligible due to the JIT. If
> that getter is not inlined at runtime, I will eat a shoe.
>
> I think the main question is still whether we want to avoid fields secretly
> springing into existence or we want the significantly more concise syntax
> of unannotated parameters. If we do want the former, then "private" versus
> "private[this]" is the next question.
>
>
> On Wed, Feb 12, 2014 at 2:34 PM, Mark Hamstra <m...@clearstorydata.com
> >wrote:
>
> > There is at least potential for performance difference with the extra
> level
> > of indirection of `private val` compared to `private[this] val`; so from
> > that perspective, `private[this]` or closing over an unannotated
> > constructor parameter is preferable to using a `private val` parameter.
> On
> > the other hand, having a field secretly spring into existence as soon an
> > unannotated parameter is closed over does present problems. On the third
> > hand, annotating constructor parameters with `private` or `private[this]`
> > is not the expected Scala way (i.e. unannotated parameters) of accessing
> > those parameters privately within their respective classes.
> >
> > In other words, I don't have a good answer, just more things to consider.
> >
> >
> >
> > On Wed, Feb 12, 2014 at 2:14 PM, Aaron Davidson <ilike...@gmail.com>
> > wrote:
> >
> > > Thanks for the clarification, Mark. So "private val" generates the
> Scala
> > > getter whereas "private[this] val" does not, and the field-ified
> > > constructor parameter mimics "private[this] val".
> > >
> > > However, the distinction between those two seems less important than
> the
> > > distinction between a constructor parameter and a field. In particular,
> > the
> > > existence of the Scala private getter won't affect serialization and
> > makes
> > > the shadowing explicit. Other than some weird uses of reflection, I'm
> not
> > > sure how the difference could cause an issue.
> > >
> > >
> > > On Wed, Feb 12, 2014 at 2:02 PM, Mark Hamstra <m...@clearstorydata.com
> > > >wrote:
> > >
> > > > It's actually a little more complicated than that, mostly due to the
> > > > difference between private and private[this]. Allow me to
> demonstrate:
> > > >
> > > > package dummy
> > > >
> > > > class Foo1(a: Int, b: Int) {
> > > > private val c = a + b
> > > > }
> > > >
> > > > class Foo2(a: Int, b: Int) {
> > > > private[this] val c = a + b
> > > > }
> > > >
> > > > class Foo3(a: Int, b: Int) {
> > > > def getB = b
> > > > }
> > > >
> > > > class Foo4(a: Int, private val b: Int) {
> > > > def getB = b
> > > > }
> > > >
> > > > class Foo5(a: Int, private[this] val b: Int) {
> > > > def getB = b
> > > > }
> > > >
> > > >
> > > > scalac Foo.scala
> > > >
> > > > Now let's look at what we've got using `javap -c -private` on the
> > > resulting
> > > > classes:
> > > >
> > > > public class dummy.Foo1 {
> > > >
> > > > private final int c;
> > > >
> > > >
> > > > private int c();
> > > >
> > > > Code:
> > > >
> > > > 0: aload_0
> > > >
> > > > 1: getfield #13 // Field c:I
> > > >
> > > > 4: ireturn
> > > >
> > > >
> > > > public dummy.Foo1(int, int);
> > > >
> > > > Code:
> > > >
> > > > 0: aload_0
> > > >
> > > > 1: invokespecial #20 // Method
> > > > java/lang/Object."<init>":()V
> > > >
> > > > 4: aload_0
> > > >
> > > > 5: iload_1
> > > >
> > > > 6: iload_2
> > > >
> > > > 7: iadd
> > > >
> > > > 8: putfield #13 // Field c:I
> > > >
> > > > 11: return
> > > >
> > > > }
> > > >
> > > > ...compare that to Foo2 and note how `c` isn't a plain field in Foo1,
> > but
> > > > has an accessor method `private int c()`:
> > > >
> > > >
> > > > public class dummy.Foo2 {
> > > >
> > > > private final int c;
> > > >
> > > >
> > > > public dummy.Foo2(int, int);
> > > >
> > > > Code:
> > > >
> > > > 0: aload_0
> > > >
> > > > 1: invokespecial #15 // Method
> > > > java/lang/Object."<init>":()V
> > > >
> > > > 4: aload_0
> > > >
> > > > 5: iload_1
> > > >
> > > > 6: iload_2
> > > >
> > > > 7: iadd
> > > >
> > > > 8: putfield #17 // Field c:I
> > > >
> > > > 11: return
> > > >
> > > > }
> > > >
> > > >
> > > > Okay? So you really need private[this] if you want to generate plain
> > > Java
> > > > fields.
> > > >
> > > > Now let's look at what happens when you close over an unannotated
> > > > constructor parameter:
> > > >
> > > >
> > > > public class dummy.Foo3 {
> > > >
> > > > private final int b;
> > > >
> > > >
> > > > public int getB();
> > > >
> > > > Code:
> > > >
> > > > 0: aload_0
> > > >
> > > > 1: getfield #14 // Field b:I
> > > >
> > > > 4: ireturn
> > > >
> > > >
> > > > public dummy.Foo3(int, int);
> > > >
> > > > Code:
> > > >
> > > > 0: aload_0
> > > >
> > > > 1: iload_2
> > > >
> > > > 2: putfield #14 // Field b:I
> > > >
> > > > 5: aload_0
> > > >
> > > > 6: invokespecial #21 // Method
> > > > java/lang/Object."<init>":()V
> > > >
> > > > 9: return
> > > >
> > > > }
> > > >
> > > >
> > > > ...which is not the same as what you get if you annotate `b` with
> > > `private
> > > > val` -- notice `private int b()`:
> > > >
> > > >
> > > > public class dummy.Foo4 {
> > > >
> > > > private final int b;
> > > >
> > > >
> > > > private int b();
> > > >
> > > > Code:
> > > >
> > > > 0: aload_0
> > > >
> > > > 1: getfield #13 // Field b:I
> > > >
> > > > 4: ireturn
> > > >
> > > >
> > > > public int getB();
> > > >
> > > > Code:
> > > >
> > > > 0: aload_0
> > > >
> > > > 1: invokespecial #18 // Method b:()I
> > > >
> > > > 4: ireturn
> > > >
> > > >
> > > > public dummy.Foo4(int, int);
> > > >
> > > > Code:
> > > >
> > > > 0: aload_0
> > > >
> > > > 1: iload_2
> > > >
> > > > 2: putfield #13 // Field b:I
> > > >
> > > > 5: aload_0
> > > >
> > > > 6: invokespecial #23 // Method
> > > > java/lang/Object."<init>":()V
> > > >
> > > > 9: return
> > > >
> > > > }
> > > >
> > > >
> > > > ...however, compare Foo3 to what you get when `b` is `private[this]
> > val`:
> > > >
> > > >
> > > > public class dummy.Foo5 {
> > > >
> > > > private final int b;
> > > >
> > > >
> > > > public int getB();
> > > >
> > > > Code:
> > > >
> > > > 0: aload_0
> > > >
> > > > 1: getfield #14 // Field b:I
> > > >
> > > > 4: ireturn
> > > >
> > > >
> > > > public dummy.Foo5(int, int);
> > > >
> > > > Code:
> > > >
> > > > 0: aload_0
> > > >
> > > > 1: iload_2
> > > >
> > > > 2: putfield #14 // Field b:I
> > > >
> > > > 5: aload_0
> > > >
> > > > 6: invokespecial #21 // Method
> > > > java/lang/Object."<init>":()V
> > > >
> > > > 9: return
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > >
> > > > On Wed, Feb 12, 2014 at 1:29 PM, Aaron Davidson <ilike...@gmail.com>
> > > > wrote:
> > > >
> > > > > Regarding styling: as we all know, constructor parameters in Scala
> > are
> > > > > automatically upgraded to "private val" fields if they're
> referenced
> > > > > outside of the constructor. For instance:
> > > > > class Foo(a: Int, b: Int) {
> > > > > def getB = b
> > > > > }
> > > > >
> > > > > In the above case, 'b' is actually a "private val" field of Foo,
> > > whereas
> > > > > 'a' never left the constructor's stack.
> > > > >
> > > > > Is this usage kosher, or should we prefer that such fields are
> marked
> > > > > "private val" explicitly if they're intended to be used outside of
> > the
> > > > > constructor? This behavior is often harmless, but it has some evil
> > > > > implications with regards to serialization and shadowing during
> > > > > inheritance. It's especially concerning when a field starts out as
> a
> > > > > constructor parameter and during a later patch becomes a field, and
> > now
> > > > > we're serializing it.
> > > > >
> > > > >
> > > > > On Mon, Feb 10, 2014 at 9:00 PM, Aaron Davidson <
> ilike...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Alright, makes sense -- consistency is more important than
> special
> > > > casing
> > > > > > for possible readability benefits. That is one of the main points
> > > > behind
> > > > > a
> > > > > > style guide after all. I switch my vote for (1) to Shivaram's
> > > proposal
> > > > as
> > > > > > well.
> > > > > >
> > > > > >
> > > > > > On Mon, Feb 10, 2014 at 4:40 PM, Evan Chan <e...@ooyala.com>
> wrote:
> > > > > >
> > > > > >> +1 to the proposal.
> > > > > >>
> > > > > >> On Mon, Feb 10, 2014 at 2:56 PM, Michael Armbrust
> > > > > >> <mich...@databricks.com> wrote:
> > > > > >> > +1 to Shivaram's proposal. I think we should try to avoid
> > > functions
> > > > > >> with
> > > > > >> > many args as much as possible so having a high vertical cost
> > here
> > > > > isn't
> > > > > >> the
> > > > > >> > worst thing. I also like the visual consistency.
> > > > > >> >
> > > > > >> > FWIW, (based on a cursory inspection) in the scala compiler
> they
> > > > don't
> > > > > >> seem
> > > > > >> > to ever orphan the return type from the closing parenthese.
> It
> > > > seems
> > > > > >> there
> > > > > >> > are two main accepted styles:
> > > > > >> >
> > > > > >> > def mkSlowPathDef(clazz: Symbol, lzyVal: Symbol, cond:
> Tree,
> > > > > >> syncBody:
> > > > > >> > List[Tree],
> > > > > >> > stats: List[Tree], retVal: Tree): Tree
> = {
> > > > > >> >
> > > > > >> > and
> > > > > >> >
> > > > > >> > def tryToSetIfExists(
> > > > > >> > cmd: String,
> > > > > >> > args: List[String],
> > > > > >> > setter: (Setting) => (List[String] =>
> > Option[List[String]])
> > > > > >> > ): Option[List[String]] =
> > > > > >> >
> > > > > >> >
> > > > > >> > On Mon, Feb 10, 2014 at 2:36 PM, Shivaram Venkataraman <
> > > > > >> > shiva...@eecs.berkeley.edu> wrote:
> > > > > >> >
> > > > > >> >> Yeah that was my proposal - Essentially we can just have two
> > > > styles:
> > > > > >> >> The entire function + parameterList + return type fits in one
> > > line
> > > > or
> > > > > >> >> when it doesn't we wrap parameters into lines.
> > > > > >> >> I agree that it makes the code a more verbose, but it'll make
> > > code
> > > > > >> >> style more consistent.
> > > > > >> >>
> > > > > >> >> Shivaram
> > > > > >> >>
> > > > > >> >> On Mon, Feb 10, 2014 at 2:13 PM, Aaron Davidson <
> > > > ilike...@gmail.com>
> > > > > >> >> wrote:
> > > > > >> >> > Shivaram, is your recommendation to wrap the parameter list
> > > even
> > > > if
> > > > > >> it
> > > > > >> >> fits,
> > > > > >> >> > but just the return type doesn't? Personally, I think the
> > cost
> > > of
> > > > > >> moving
> > > > > >> >> > from a single-line parameter list to an n-ine list is
> pretty
> > > > high,
> > > > > >> as it
> > > > > >> >> > takes up a lot more space. I am even in favor of allowing a
> > > > > parameter
> > > > > >> >> list
> > > > > >> >> > to overflow into a second line (but not a third) instead of
> > > > > spreading
> > > > > >> >> them
> > > > > >> >> > out, if it's a private helper method (where the parameters
> > are
> > > > > >> probably
> > > > > >> >> not
> > > > > >> >> > as important as the implementation, unlike a public API).
> > > > > >> >> >
> > > > > >> >> >
> > > > > >> >> > On Mon, Feb 10, 2014 at 1:42 PM, Shivaram Venkataraman
> > > > > >> >> > <shiva...@eecs.berkeley.edu> wrote:
> > > > > >> >> >>
> > > > > >> >> >> For the 1st case wouldn't it be better to just wrap the
> > > > parameters
> > > > > >> to
> > > > > >> >> >> the next line as we do in other cases ? For example
> > > > > >> >> >>
> > > > > >> >> >> def longMethodName(
> > > > > >> >> >> param1,
> > > > > >> >> >> param2, ...) : Long = {
> > > > > >> >> >> }
> > > > > >> >> >>
> > > > > >> >> >> Are there a lot functions which use the old format ? Can
> we
> > > just
> > > > > >> stick
> > > > > >> >> >> to the above for new functions ?
> > > > > >> >> >>
> > > > > >> >> >> Thanks
> > > > > >> >> >> Shivaram
> > > > > >> >> >>
> > > > > >> >> >> On Mon, Feb 10, 2014 at 11:33 AM, Reynold Xin <
> > > > > r...@databricks.com>
> > > > > >> >> wrote:
> > > > > >> >> >> > +1 on both
> > > > > >> >> >> >
> > > > > >> >> >> >
> > > > > >> >> >> > On Mon, Feb 10, 2014 at 1:34 AM, Aaron Davidson <
> > > > > >> ilike...@gmail.com>
> > > > > >> >> >> > wrote:
> > > > > >> >> >> >
> > > > > >> >> >> >> There are a few bits of the Scala style that are
> > > > underspecified
> > > > > >> by
> > > > > >> >> >> >> both the Scala
> > > > > >> >> >> >> style guide <http://docs.scala-lang.org/style/> and
> our
> > > own
> > > > > >> >> >> >> supplemental
> > > > > >> >> >> >> notes<
> > > > > >> >> >> >>
> > > > > >> >> >> >>
> > > > > >> >>
> > > > > >>
> > > > >
> > >
> https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide
> > > > > >> >.
> > > > > >> >> >> >> Often, this leads to inconsistent formatting within the
> > > > > >> codebase, so
> > > > > >> >> >> >> I'd
> > > > > >> >> >> >> like to propose some general guidelines which we can
> add
> > to
> > > > the
> > > > > >> wiki
> > > > > >> >> >> >> and
> > > > > >> >> >> >> use in the future:
> > > > > >> >> >> >>
> > > > > >> >> >> >> 1) Line-wrapped method return type is indented with two
> > > > spaces:
> > > > > >> >> >> >> def longMethodName(... long param list ...)
> > > > > >> >> >> >> : Long = {
> > > > > >> >> >> >> 2
> > > > > >> >> >> >> }
> > > > > >> >> >> >>
> > > > > >> >> >> >> *Justification: *I think this is the most commonly used
> > > style
> > > > > in
> > > > > >> >> Spark
> > > > > >> >> >> >> today. It's also similar to the "extends" style used in
> > > > > classes,
> > > > > >> with
> > > > > >> >> >> >> the
> > > > > >> >> >> >> same justification: it is visually distinguished from
> the
> > > > > >> 4-indented
> > > > > >> >> >> >> parameter list.
> > > > > >> >> >> >>
> > > > > >> >> >> >> 2) URLs and code examples in comments should not be
> > > > > line-wrapped.
> > > > > >> >> >> >> Here<
> > > > > >> >> >> >>
> > > > > >> >> >> >>
> > > > > >> >>
> > > > > >>
> > > > >
> > > >
> > >
> >
> https://github.com/apache/incubator-spark/pull/557/files#diff-c338f10f3567d4c1d7fec4bf9e2677e1L29
> > > > > >> >> >> >> >is
> > > > > >> >> >> >> an example of the latter.
> > > > > >> >> >> >>
> > > > > >> >> >> >> *Justification*: Line-wrapping can cause confusion when
> > > > trying
> > > > > to
> > > > > >> >> >> >> copy-paste a URL or command. Can additionally cause IDE
> > > > issues
> > > > > >> or,
> > > > > >> >> >> >> avoidably, Javadoc issues.
> > > > > >> >> >> >>
> > > > > >> >> >> >> Any thoughts on these, or additional style issues not
> > > > > explicitly
> > > > > >> >> >> >> covered in
> > > > > >> >> >> >> either the Scala style guide or Spark wiki?
> > > > > >> >> >> >>
> > > > > >> >> >
> > > > > >> >> >
> > > > > >> >>
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >> --
> > > > > >> --
> > > > > >> Evan Chan
> > > > > >> Staff Engineer
> > > > > >> e...@ooyala.com |
> > > > > >>
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
