Java: public default constructor is error-prone

[LouisCAD]

Hi!

I am currently using flatbuffers to both hold default values and allow to sync user themes (for an Android Wear Watch Face) between the handset and the wearables. I always need a Themeobject (which is a flatbuffers Table) available to get the default values if the user hasn't set any theme. How to create an empty object is not clear.

From the code (and I tried to confirm), creating a new object initially (new Theme()) is possible, and creates one with a null``ByteBuffer, leading to a NullPointerException as soon as you call an accessor.

private final Theme mTheme = new Theme(); // Should work but leads to NPEs on accesses.

I tried calling this:

private final Theme mTheme = Theme.getRootAsTheme(ByteBuffer.allocate(0)); // Doesn't work

But got an IndexOutOfBoundsException when accessing a property.

Finally, this code worked:

private final Theme mTheme = Theme.getRootAsTheme(ByteBuffer.allocate(4));

IMHO, the first attempt (new YourFlatbuffersTable()) should work without pending NPEs, and getRootAsYourFlatbuffersTable() should be only needed when you have a real flatbuffers binary.

I suggest to address this issue by making the __offset(…) method in Table class check for bb nullability, and return 0 if it is, so the calling accessor/mutator skips to default value/false without trying to call any method on bb.

This would just need one line at the beginning of the method:

protected int __offset(int vtable_offset) {
    if (bb == null) return 0; // This line I added allows empty flatbuffers
    int vtable = bb_pos - bb.getInt(bb_pos);
    return vtable_offset < bb.getShort(vtable) ? bb.getShort(vtable + vtable_offset) : 0;
  }

Also, it should be clearly documented (especially for network apps) that accesses can throw IndexOutOfBoundsException if wrong data it in the ByteBuffer for current implementation, and IMHO, future implementations should have a public static boolean checkIntegrity(ByteBuffer _bb) method generated in each table class, designed to be called before getRootAsYourFlatbuffersTable(ByteBuffer _bb, YourFlatbuffersTable obj) (hence the static keyword) which would prevent someone causing a crash to your app by corrupting network (or shared disk) data. Maybe this is worth a new issue?

[ghost]

Classes like Theme are there as a handle to access already populated FlatBuffers, they are not intended to construct/mutate buffers. Check the tutorial on how to create a buffer.

C++ has a Verifier to check the integrity of buffers (necessary because there are no bound checks). In Java, if you can be reading corrupted buffers, currently IndexOutOfBoundsException is indeed how you'll hear about it.

[LouisCAD]

So a decision must be taken. Either add the line of code I suggested so creating an empty flatbuffers is easy as ABC, or add a private constructor to each generated class so nobody can try to create a broken flatbuffers.

I would personally prefer the first option to don't add complexity where not needed.

How to create an empty Flatbuffers to get default values:

First solution:

mTheme = new Theme(); // Requires only one extra line in Table#__offset() to return 0 if bb is null.

Second solution:

FlatBufferBuilder themeBuilder = new FlatBufferBuilder(0);
Theme.startTheme(themeBuilder);
final int o = Theme.endTheme(themeBuilder);
Theme.finishThemeBuffer(themeBuilder, o);
mTheme = Theme.getRootAsTheme(themeBuilder.dataBuffer());

I guess it's pretty obvious which one requires less boilerplate from developers…

[Lakedaemon]

As @GWO said the generated code is there to handle already populated flatbuffers (in java, bytebuffer rightly written by the flatbuffers generated code....ByteBuffer.allocate(4) might not be a rightly built bytebuffer; it might not work for complex flatbuffers that will access bytes fartyher than the first 4 bytes)

You should probably handle your problem differently :
In your case, I would probably use a ThemeWrapper class that would wrap a private Theme class
The Them class is used to read/write flatbuffers while the ThemeWrapper class hides all the Theme method and exposes only the api you need

handling a null bytebuffer (and defaults) could then be done at the ThemeWrapper stage

[Lakedaemon]

Also, if you code for android and if you use kotlin, you might want to use the kotlin code generator implemented there #1190
(If you do, I would love to get feedback), it is much easier to use, terser and you get null safety

[LouisCAD]

I don't use kotlin yet, but I don't see the benefit of flatbuffers default values if I have to write a boilerplate friendly class myself, which anyway, can't read flatbuffers default values as it will always get an NPE. I mean, if I create other tables, I have to rewrite each default values for each table in a wrapper class? I don't get how you find it more practical that the ability to instantiate a working empty flatbuffers table with the new keyword…

Please, tell me, if I want to use it in java, I have to write another class manually and copy paste all the default values and created delegates for the class under the wrapper, or stop using java? Don't you think it deviates from flatbuffers philosophy which is closer to Don't Repeat Yourself and which aims to keep things simple?

Anyway, I'm lucky that it's so easy to edit flatbuffers behavior as there's no read-only dependency, so I added the line I was talking about in the Table class which checks if bb is null, and it works flawlessly.
I'm ok to do a PR if you think it's ok to add a null check in the __offset(…) method.

[Lakedaemon]

First, please wait for what @GWO has to say. I happened to write the
kotlin code generator for flatbuffers but I don't have his wisdom when
it comes to the library, so I can only give you my opinion (which is not
official) :

Le 15/07/2016 15:46, Louis Cognault a écrit :

I don't use kotlin yet, but I don't see the benefit of flatbuffers
default values if I have to write a boilerplate friendly class myself,
which anyway, can't read flatbuffers default values as it will always
get an NPE.

The problem is that, at the moment, the generated classes are designed
to read from an existing (not null) message

I mean, if I create other tables, I have to rewrite each default
values for each table in a wrapper class?

you want to implement your defaults in the schema file and only use that.
The best solution I know at the moment is the first one that you
suggested (create a rightly built bytebuffer)

I don't get how you find it more practical that the ability to
instantiate a working empty flatbuffers table with the |new| keyword

Please, tell me, if I want to use it in java, I have to write another
class manually and copy paste all the default values and created
delegates for the class under the wrapper, or

it would work but it would probably be a lot more work and hassle as
using your first solution for a complex flatbuffer

stop using java?

no, the problem will be the same with kotlin. Sorry for misleading you.

Don't you think it deviates from flatbuffers philosophy which is
closer to Don't Repeat Yourself and which aims to keep things simple?

I see your point and I think that I understand your need.

Anyway, I'm lucky that it's so easy to edit flatbuffers behavior as
there's no read-only dependency, so I added the line I was talking
about in the |Table| class which checks if |bb| is null, and it works
flawlessly.

This should work for scalars, strings and vectors in a Table but what
about structs though ?
They are inlined in a flatbuffer and don't have defaults...
methinks the generated code will try to look inside the bytebuffer to
fetch the values

I'm ok to do a PR if you think it's ok to add a null check in the
|__offset(…)| method.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#3945 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGTB3d0gOjSgsAYisPROYdJOIwuxHXSks5qV489gaJpZM4JM6B5.

[LouisCAD]

@Lakedaemon Don't structs default to null when not set? If yes, there should be a way to return null if bb is null?

[Lakedaemon]

No, they don't. A struct is never null. They are inlined. If the root of your flatbuffer is a struct that holds an int, you get a bytebuffer with at least 4 bytes (might even be more because of other flatbuffer features (file identifyer)) .

[LouisCAD]

I'll try adding a struct to my flatbuffers to see how this could be addressed

[LouisCAD]

@Lakedaemon Where did you see that structs are never null? From what I see by reading generated code, they can be, and the line I added is struct safe in this regard.

[Lakedaemon]

My bad, you are right (I tried to be of help but failed miserably, sorry).

1. In a Table, a Struct field can be null.
2. as only tables can be root types (and not struct... I had forgotten
   about that),
   it looks like you can get your defaults for an empty flatbuffer with
   your single line.

Le 16/07/2016 16:47, Louis Cognault a écrit :

@Lakedaemon https://github.com/Lakedaemon Where did you see that
structs are never null? From what I see by reading generated code,
they can be, and the line I added is struct safe in this regard.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3945 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGTBwjVBDRfOzl9yOAUr5No1k1brGljks5qWO8dgaJpZM4JM6B5.

[LouisCAD]

@Lakedaemon It made me check that it would work properly with structs, so thanks! Last thing to check is for unions and their type. Do you have an idea?

[Lakedaemon]

Le 16/07/2016 18:02, Louis Cognault a écrit :

@Lakedaemon https://github.com/Lakedaemon Thanks to you, I checked
that it would work properly with structs, so thanks! Last think to
check is for unions and their type. Do you have an idea?

Apparently, with java generated code, with offset = 0
unions return null and unionType returns 0 = NONE

So, I guess that it would work.
Still, modifying the offset function that way would incur a slight
performance hit

It's trading off convenience against performance :
you save a few lines of code and a small bytebuffer allocation when
creating defaults at start (once),
but you pay the price every time you access a flatbuffer field (and some
people access million+ times their flatbuffers)

Also, another reason not to do it this way (though it works) is that you
lose null safety :
The kotlin generator is garanted by the language and the compiler to be
null safe.
If it was allowed for the bytebuffer to be null, I would have to put
null checks everywhere to make the generated code compile (and that
would be another performance hit).

The java code generator doesn't have this problem as java doesn't care
at all about null safety
(given a Table variable, it could be null or not null, you have no way
to know till you test it)
(in Kotlin, you have Table that are garanted to be not null and Table?
that can be null)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3945 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGTB6Jw491Z5BTqQQuB5pA4CftKPBgSks5qWQB-gaJpZM4JM6B5.

[LouisCAD]

@Lakedaemon I don't think checking if a reference points to null can be considered as overhead. I mean, it's a very cheap operation, which maybe kotlin runtime overpasses 😉 . I'd benchmark without and without if I was afraid of a performance hit though.

And about kotlin, this is only about java, so maybe if this problem is present in kotlin too, there'd be another way to address it, by hiding the FlatbuffersBuilder calling code in the constructor for example?

BTW, glad to see it'd work on unions too and that it's absolutely safe for the whole flatbuffers grammar

[ghost]

Sorry, you can't construct FlatBuffers from empty using the accessor API. You'll have to use building API for that instead.

[LouisCAD]

@gwvo So can it be simplified for cases when you want to leave all default values untouched?

What about something like the following that would generate all the boilerplate code needed to build a flatbuffer with just default values?

Code that the programmer needs to write

mTheme = Theme.createDefaultTheme();
mMonster = Monster.createDefaultMonster();

Generated boilerplate code

public class Theme {
    private Theme() {}

    public static Theme createDefaultTheme() {
        FlatBufferBuilder themeBuilder = new FlatBufferBuilder(0);
        Theme.startTheme(themeBuilder);
        final int o = Theme.endTheme(themeBuilder);
        Theme.finishThemeBuffer(themeBuilder, o);
        return Theme.getRootAsTheme(themeBuilder.dataBuffer());
    }
   ...
}

public class Monster {
    private Monster() {}

    public static Monster createDefaultMonster() {
        FlatBufferBuilder monsterBuilder = new FlatBufferBuilder(0);
        Monster.startMonster(monsterBuilder);
        final int o = Monster.endMonster(monsterBuilder);
        Monster.finishMonsterBuffer(monsterBuilder, o);
        return Monster.getRootAsMonster(monsterBuilder.dataBuffer());
    }
    ...
}

[ghost]

I don't see what the point is to go all the way thru the trouble of serializing an empty FlatBuffer just to access default values?

[LouisCAD]

@gwvo I'm not serializing any flatbuffers with the line I added in the Table class in my project, but I'm trying to find an alternative as you seems to dislike the null check.

Maybe you have a better solution to access default values as I do when the user didn't configured any theme yet?

[ghost]

I think the solution is to write the "boilerplate" code you showed.. I don't think this functionality is common enough to have the code generator take care of it.

[LouisCAD]

Mine is to edit the Table.java to add a null check. I think it'll be hard for us to agree if you keep advocating for boilerplate with no visible reason, which seems to counter flatbuffers policy.
BTW, we have no real way to know if some feature will be used, but adding it is pretty simple, and shouldn't negatively impact Flatbuffers users.

And about use cases:
Any app that uses Flatbuffers for some configuration having defaults that may be left untouched, or that may need to work with default values until a configuration is fetched from an async, potentially failing operation such as from network.
Think IOT, wearables...

[LouisCAD]

It seems that you were a little uncomfortable with the possible performance hit of adding a null check in the Table class (if (bb == null) return 0; // This line I added allows empty flatbuffers).

1. We are talking about java. People who want brute performance will choose Go or C++, not java.
2. I did a little research about this, and the "performance hit" that a null check adds can in no way be called overhead. In this stackoverflow answer, a guy did 2,000,000,000 null checks, and the worst he could get was less than 3ns for a null check (2.574).
3. Considering java apps are probably not games, but more likely to be Android UI/background apps, or server apps, how could a 3ns impact performance in any way? Even if the null check is done multiple times, if the developer did this on the UI thread in and Android app, it would need to be done 5,333,333 times to cause a frame drop by itself. Of course in Android, heavy jobs are not done on the UI thread if the developer follows best practices, so I think we should give this a try.
4. Finally, it would solve the problem of the default constructor which instead of creating a broken object, would create a default one, which would work.

I'm doing the 1-line-of-code pull request, hope it'll be accepted

[ghost]

Efficiency is the #1 priority in FlatBuffers, even in languages like Java where that isn't easy. Adding an if-check in a very frequently executed part of the code is therefore not to be taken lightly.

Now if that if-check would be a major improvement to the API, it could be considered, but in this case it appears the use of it is to save a single person, you, to have to write some extra code. That is not enough reason to make everyone else's code a tiny bit slower for no clear benefit.

[Lakedaemon]

Also, how can 2,000,000,000 null checks only take 3ns when a cpu only has about 3,000,000,000 cycles in a second ?

Those null checks in the test might have been ignored by the compiler and I wouldn't trust that benchmark...

[LouisCAD]

It's an average per loop, done with 2Billion loops. So multiply 3ns by
2Billion

On Mon, Aug 22, 2016, 11:24 PM Olivier Binda notifications@github.com
wrote:

Also, how can 2,000,000,000 null checks only take 3ns when a cpu only has
about 3,000,000,000 cycles in a second ?

Those null checks in the test might have been ignored by the compiler and
I wouldn't trust that benchmark...

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#3945 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGpvBXsVrd5RHA4hB-dRaQ58BoR85rBSks5qihOTgaJpZM4JM6B5
.