PSA: Don't initialize default values in Java

[Andrew Grieve]

Turns out

class Foo {

  boolean bar;

}

results in fewer compiled bytes than:

class Foo {

  boolean bar = false;

}

There's an explanation as to why this is the case here:

http://stackoverflow.com/questions/14663859/default-variables-values-vs-initialization-with-default

I'd like to at some point add and Android Lint warning for this, but for now at least wanted to ensure everyone knew this was the case.

[Peter Kasting]

On Wed, Oct 5, 2016 at 10:38 AM, Andrew Grieve <agr...@chromium.org> wrote:

Turns out

class Foo {

  boolean bar;

}

results in fewer compiled bytes than:

class Foo {

  boolean bar = false;

}

There's an explanation as to why this is the case here:

http://stackoverflow.com/questions/14663859/default-variables-values-vs-initialization-with-default

That explanation is interesting.

The example program given in the answer wouldn't work the same as-is in C++, because it's doing a polymorphic call from the superclass constructor, which apparently calls the subclass method in Java but would call the superclass method in C++.  I don't think there's a way in C++ to have a time when an object's fields are accessible but that object's constructor has not run, in which case it seems like C++ would not have a gotcha like this.  But I'm not sure.

PK

[Torne (Richard Coles)]

Andrew, I'm not sure your post here is very clear - the stackoverflow explanation you linked to is explaining that there is a significant gotcha to *not* initialising default values in certain cases, because (unlike in C++) in Java it's possible do polymorphic calls in the constructor and they actually resolve to the subclass method, as Peter notes.

So I think your advice here is a bit unclear: you're advising people *not* to initialise default values because it saves bytes in the compiled binary (which is indeed true), but linking to a source that explains that sometimes you actually *must* initialise the default value because not doing so will change the program's behaviour, and that this is outside your direct control (because it depends on the subclass's behaviour).

I support trying to save bytes where we can, but this is a bit of an alarming gotcha to have; I'm not sure if it's possible to come up with a clear set of guidelines on when we should/shouldn't be applying this optimisation.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Anthony Berent]

More fun with this. If you build and run:

class Superclass {

    public Superclass() {

        someMethod();

    }

    void someMethod() {}

}

class Subclass extends Superclass {

    private final int constant = 5;

    private int variable = 5;

    public Subclass() {

        System.out.println("constant: " + constant);

        System.out.println("variable: " + variable);

    }

    @Override void someMethod() {

        System.out.println("constant in someMethod: " + constant);

        System.out.println("variable in someMethod: " + variable);

    }

}

public class Test {

    public static void main(String[] args) {

        new Subclass();

    }

}

It prints:

constant in someMethod: 5

variable in someMethod: 0

constant: 5

variable: 5

Whether or not we initialise default values I think the real problem is that calling a non-final method in a constructor gives non-intuitive behaviour. Maybe, whatever we do about warning about initialising default values, we should  (if Android Lint can do it) warn about calling any non-final methods in constructors.

[Colin Blundell]

As an outsider to Java, Anthony's conclusion was mine from reading this thread.

[Dmitry Skiba]

I think the advice is only about explicitly initializing with default values. I.e. 'false' is default for boolean, but explicitly writing 'boolean foo = false' results in more bytes.

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Andrew Grieve]

Thanks for pointing this out Torne.

The point of the stackoverflow link was to explain why the compiler can't just optimize this away for us. But, also to show that the semantic difference is one that is a terribly confusing and likely never the intended behaviour.

[Colin Blundell]

What Anthony is pointing out, and I agree with, is that in the example given the behavior is subtle and surprising even in the case where the variables are explicitly initialized. i.e., as a non-Java programmer, my reading of the stack overflow thread is that calling a method in the superclass constructor that is overridden by a subclass to mutate some instance state is an anti-pattern, full stop. Maybe this is common in Java though...if Java programmers have grown to rely on this behavior, then it seems like we can't actually give the recommendation in the OP (which is kind of what torne@ was saying).

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Colin Blundell]

That's also the case that the stack overflow thread is about. It's the subclass' overriding method that sets to a value other than the default.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Anthony Berent]

BTW I will leave as an exercise for the reader the question of what happens if you change the program to:

class Superclass {

    public Superclass() {

        someMethod();

    }

    void someMethod() {}

}

class Subclass extends Superclass {

    private int variable = 5;

    private final int constant = variable;

[Torne (Richard Coles)]

On Thu, 6 Oct 2016 at 17:39 Colin Blundell <blun...@chromium.org> wrote:

What Anthony is pointing out, and I agree with, is that in the example given the behavior is subtle and surprising even in the case where the variables are explicitly initialized. i.e., as a non-Java programmer, my reading of the stack overflow thread is that calling a method in the superclass constructor that is overridden by a subclass to mutate some instance state is an anti-pattern, full stop. Maybe this is common in Java though...if Java programmers have grown to rely on this behavior, then it seems like we can't actually give the recommendation in the OP (which is kind of what torne@ was saying).

It is an antipattern, but it's one that people write without knowing it sometimes. People don't declare methods final very often, whether they expect them to be overridden or not. Pretty much the only way to make sure you don't write this antipattern is to never allow "this" to escape from the constructor, which includes not calling any methods on this either. (unless everything you call is final and also does not allow "this" to escape).

[Colin Blundell]

In the case where someone writes a subclass like this without knowing it and the ivar is initialized explicitly (either in the superclass or in the subclass), is it not just as likely that that someone will be surprised by the fact that the ivar is back to its default value at the end of initializing the object? Or are people actually familiar with this in Java and would expect that? (in which case it seems strange to even structure the code that way in the first place...)

Anyway, I'm basically bikeshedding at this point :).

[Torne (Richard Coles)]

On Thu, 6 Oct 2016 at 18:14 Colin Blundell <blun...@chromium.org> wrote:

In the case where someone writes a subclass like this without knowing it and the ivar is initialized explicitly (either in the superclass or in the subclass), is it not just as likely that that someone will be surprised by the fact that the ivar is back to its default value at the end of initializing the object?

It's never possible for the field to be initialised explicitly "either in the superclass or the subclass" - fields can only be initialised where they are declared. The subclass can't have an initialiser for a field that was defined in the superclass. The field also doesn't go back to its default value at the *end* of initialising the object, but at the beginning of initialisation for the class that defined that field.

If I'm misinterpreting your question then sorry for the verbose explanation here, but the order of operations in general for "class SubClass extends class SuperClass", is:

1) All fields of the object start off with the spec-specified default values: false for bools, zero for numbers, null for objects.

2) All the fields that have an intiailiser in SuperClass get initialised to their specified values

3) SuperClass's constructor runs

4) All the fields that have an initialiser in SubClass get initialised to their specified values

5) SubClass's constructor runs

So, for SuperClass, it doesn't matter whether it explicitly initialises bools to false or not - no code runs inbetween the time that the default initialisation happens and the explicit initialisation. The case that can be surprising is that *if* SuperClass's constructor calls a method that's overridden in SubClass, then:

a) the method will see all the fields of SubClass still being default-initialised, because SubClass's field initialisers and constructor haven't yet run. This might *already* just break your program, regardless of field initialisers being involved at all. This is the case Anthony was pointing out, where the code might be broken whether you explicitly initialise fields or not, and so isn't really related to the PSA here, just a related Java gotcha that doesn't apply to C++.

b) any change to SubClass's fields during that method will be overwritten if *either* SubClass's field initialisers, or SubClass's constructor, set that field to another value. This is the case where there's a difference between "bool foo = false;" and "bool foo;" in the subclass - but note that this is not actually different to the constructor just containing "foo = false;" at all.

Or are people actually familiar with this in Java and would expect that? (in which case it seems strange to even structure the code that way in the first place...)

Lots of Java code does call non-final methods during constructors, and usually this just doesn't hit either of those problems, so people probably don't think about it.

Android has quite a few examples of this, and sometimes it does cause issues: for example, the Android class "View" calls the non-final method setOverScrollMode on itself during its constructor, and a number of View subclasses (including WebView) *do* override setOverScrollMode, and have to be very careful to make sure that their overrides of setOverScrollMode don't do something that causes a) or b) to be a problem. However, this pretty much just has to be discovered *when implementing a View subclass* - the only way to know this is possible is to read the source code for View, or for your app to crash at runtime because you did cause one of these problems in your subclass, and it's always possible for code that is *currently* working to be broken if the superclass's constructor changes to call a new method that it didn't call previously :(

[Colin Blundell]

This makes sense, thanks. As an outsider to Java, your (a) below already seems like such a footgun that I would be inclined to make a Chromium policy like the following:

- Superclasses should document non-final methods called in their constructor.

- Subclasses should avoid manipulating their defined ivars in their overrides of methods called in the superclass' constructor.

If we had that policy then we could employ the optimization mentioned in the PSA at will, since it wouldn't have any observable effect on program behavior.

If that policy is untenable (which I would assume it is), then I think that at least avoiding footgun (b) makes sense, which implies doing the *opposite* of the PSA and actually *requiring* that classes explicitly initialize their ivars. That way at least people can safely make the assumption that when *their* constructor runs the instance's ivars have the values that they were (explicitly) initialized to.

[Colin Blundell]

Sorry by "avoiding footgun (b)" I more precisely meant "avoiding the uncertainty of not knowing what value a subclass' ivar has at the time that its constructor is run." This is not avoiding (b) but actually enforcing (b).

[Torne (Richard Coles)]

If you actually want to make that gotcha impossible, then it's, sadly, worse than just "document non-final methods called in constructors", because:

1) you have to apply that transitively: also all non-final methods called by final methods called in constructors, and so on

2) you also have to apply that to any calls in the constructor to *other* functions (even final ones) outside the current class that take "this" as a parameter, because the other function might well call a non-final method on "this". And then apply *that* transitively as well.

I'm not sure that trying to have a policy about constructor behaviour is going to help a lot, really.

I don't really have a conclusion here personally; I do understand why we want to save all the bytes we can on android (binary size matters a lot), and I think a proguard optimisation that strips these redundant initialisers is interesting to pursue, and I don't even think it's that likely that it will break anything in practise... but it does make my skin crawl a bit to propose transforms that might affect correctness.

[Andrew Grieve]

On Fri, Oct 7, 2016 at 11:58 AM, Torne (Richard Coles) <to...@chromium.org> wrote:

If you actually want to make that gotcha impossible, then it's, sadly, worse than just "document non-final methods called in constructors", because:

1) you have to apply that transitively: also all non-final methods called by final methods called in constructors, and so on

2) you also have to apply that to any calls in the constructor to *other* functions (even final ones) outside the current class that take "this" as a parameter, because the other function might well call a non-final method on "this". And then apply *that* transitively as well.

I'm not sure that trying to have a policy about constructor behaviour is going to help a lot, really.

I don't really have a conclusion here personally; I do understand why we want to save all the bytes we can on android (binary size matters a lot), and I think a proguard optimisation that strips these redundant initialisers is interesting to pursue, and I don't even think it's that likely that it will break anything in practise... but it does make my skin crawl a bit to propose transforms that might affect correctness.

This scares me too. That's why I'd like to add a lint warning and not a proguard optimization.

 

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Jeffrey Yasskin]

And this is why C++ has the footgun that calls to virtual functions during a base class constructor get the implementation defined in the base class. :)

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Colin Blundell]

D'oh, of course :P.

[Holger]

Has anyone tested this with an apk that is proguarded?
I just wanted to see how much this could save in Chrome, and on a proguarded debug build I removed slightly more than 40 default initializers (which set variables to 0 or false)
The result however shows that the apk size actually increased (possibly due to proguard being better at optimizing initialized variables, though that's just a theory)

ls -al
Before: -rw-r--r-- 2 myusername domain^users 64690123 Oct 12 11:21 out/GN-Debug/apks/AlteredChromePublic.apk
After: -rw-r--r-- 2 myusername domain^users 64711075 Oct 12 11:44 out/GN-Debug/apks/AlteredChromePublic.apk
--> The apk size increased by a good 20 kB
--> Has anyone actually tried this on a Release build of Chromium and seen benefits? Are there some gotchas I should be aware of? (E.g. I did this for static and non-static members alike. Was that a bad idea?)

[Vaibhav Pithadiya]

Do not disturb, ,
.

[Andrew Grieve]

When I first tested this theory out, I did it on a release binary and did see the (uncompressed) dex size shrink a bit. Proguard is a crazy thing though, so I suppose it's possible that there's this much randomness?

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Eric Stevenson]

I think that the subtleties discussed in this thread may be orthogonal to whether or not we should initialize default values in Java.

I've created a CL (https://codereview.chromium.org/2548013002/) that removes most field initializers in Java, saving us 7.62 kb. We can update the style guide (https://sites.google.com/a/chromium.org/dev/developers/coding-style/java) to reflect this preference - does anyone have any objections to me landing this?

[Holger]

Just out of curiosity: 7.62 kb on a proguarded apk or on a multidexed apk?

On Wednesday, December 7, 2016 at 11:30:55 AM UTC-8, Eric Stevenson wrote:

I think that the subtleties discussed in this thread may be orthogonal to whether or not we should initialize default values in Java.

I've created a CL (https://codereview.chromium.org/2548013002/) that removes most field initializers in Java, saving us 7.62 kb. We can update the style guide (https://sites.google.com/a/chromium.org/dev/developers/coding-style/java) to reflect this preference - does anyone have any objections to me landing this?

On Fri, Oct 14, 2016 at 10:54 AM, Andrew Grieve <agr...@chromium.org> wrote:

When I first tested this theory out, I did it on a release binary and did see the (uncompressed) dex size shrink a bit. Proguard is a crazy thing though, so I suppose it's possible that there's this much randomness?

On Wed, Oct 12, 2016 at 3:01 PM, Vaibhav Pithadiya <vaibhav....@gmail.com> wrote:

Do not disturb, ,
.

On Thu, 13 Oct 2016 12:23 AM 'Holger' via Chromium-dev, <chromi...@chromium.org> wrote:

Has anyone tested this with an apk that is proguarded?
I just wanted to see how much this could save in Chrome, and on a proguarded debug build I removed slightly more than 40 default initializers (which set variables to 0 or false)
The result however shows that the apk size actually increased (possibly due to proguard being better at optimizing initialized variables, though that's just a theory)

ls -al
Before: -rw-r--r-- 2 myusername domain^users 64690123 Oct 12 11:21 out/GN-Debug/apks/AlteredChromePublic.apk
After: -rw-r--r-- 2 myusername domain^users 64711075 Oct 12 11:44 out/GN-Debug/apks/AlteredChromePublic.apk
--> The apk size increased by a good 20 kB
--> Has anyone actually tried this on a Release build of Chromium and seen benefits? Are there some gotchas I should be aware of? (E.g. I did this for static and non-static members alike. Was that a bad idea?)

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Anton Vayvod]

I second the opinion voiced on the code review (that I copied below) that the explicit initialization even with the default values may improve readability:

"Personally, I also find that removing the initialization makes the code less

readable, since it forces the reader to hunt around for where it is initialized,

whereas e.g. explicitly initializing an object to null helpfully implies that it

is nullable."

To me it also makes reading the mixed C++/Java codebase easier: if I see a C++ int variable that is not initialized my instinct is it's a mistake. In Java I have to remember that it's initialized and whether the default value of 0 is correct.

I'd support removing the explicit initialization with default values if the binary size wins would justify it.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Eric Stevenson]

Replying inline:

Just out of curiosity: 7.62 kb on a proguarded apk or on a multidexed apk?

 

This is for proguarded (is_java_debug = false) chrome_public_apk. The uncompressed size of classes.dex shrinks by 7.62 kb after applying the patch.

"Personally, I also find that removing the initialization makes the code less

readable, since it forces the reader to hunt around for where it is initialized,

whereas e.g. explicitly initializing an object to null helpfully implies that it

is nullable."

I agree that this affects readability - but only marginally. Seeing a field declared but not explicitly initialized = field is initialized to default value. Regardless of whether or not you explicitly initialize the field at the time of declaration, this value could be overridden in the constructor. Could you clarify what you mean by this?

To me it also makes reading the mixed C++/Java codebase easier: if I see a C++ int variable that is not initialized my instinct is it's a mistake. In Java I have to remember that it's initialized and whether the default value of 0 is correct.

Agreed, but I think the binary size savings are worth it.

 I'd support removing the explicit initialization with default values if the binary size wins would justify it.

Agreed! Maybe some others working in this area will chime in.

[Andrew Grieve]

I actually find the opposite is true in terms of readability. If I see it explicitly initialized to a default value, I think "Oh, that's odd, I wonder why that's necessary". 

I don't think it's worth arguing over something that's so clearly subjective though. We should just do what saves bytes (as well as some runtime cost).

[Holger]

Nice, thanks for confirming Eric! :)

[Eric Stevenson]

This is now completed: https://crrev.com/2b97db470c6419fe4f15daf091789bba251ec437.

--