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 |
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>
