[JS] FlexBuffers Support #5973

mzaks · 2020-06-16T15:21:04Z

This PR introduces FlexBuffers support for JavaScript.

…lder Dart version

[] operator throws a very descriptive exception in case of a bad key.

js/flexbuffers.js

aardappel

Thanks for creating this! Have a few comments though :)

js/flexbuffers.js

aardappel · 2020-06-18T16:30:26Z

js/flexbuffers.js

+};
+
+flexbuffers.builder = (size = 2048) => {
+  let buffer = new ArrayBuffer(size > 0 ? size : 2048);


Just let the ArrayBuffer constructor throw something if someone passes a negative size?

Oh there is another purpose in this expression. The grow buffer algorithm will not work properly if the ArrayBuffer size is 0. So I would rather keep it here then checking each time I need to grow the buffer. It is also called just once per buffer creation so IMHO no big performance penalty.

js/flexbuffers.js

aardappel · 2020-06-18T16:35:35Z

js/flexbuffers.js

+    if (finished) {
+      throw "Adding values after finish is prohibited";
+    }
+    if (stackPointers.length !== 0 && stackPointers[stackPointers.length - 1].isVector === false) {


why not do this check as a map is being serialized, which is much cheaper. See C++ implementation.

Because IMHO it is better to keep cause and effect close together. This way the exception is thrown when I add the value so I will know how to fix it instantly. If the exception is thrown when the map is being serialized, I know I did something wrong, but I have no idea what exactly so I will have to manually go through my builder code and search for one or multiple bugs.

for errors that are not likely to be frequent, I'd rather have a focus on performance.

aardappel · 2020-06-18T16:37:52Z

js/flexbuffers.js

+        return;
+      }
+      if (cache && indirectIntCache.hasOwnProperty(value)) {
+        stack.push(indirectIntCache[value]);


there's a cache for indirect int values? why?

It's based on following API:

addInt: function(value, indirect = false, cache = false)

If I want to add an int indirectly, I can also cache it so if I add another int with the same value it will be reused. I think there is similar strategy in C++ but there user need to store the offset and provide it. It is faster but not as safe as users might provide a wrong offset. As JS is in general a language where it is easier to make mistakes ;) I chose to provide an API which provides smaller surface for errors.

Yes, but these lookups are expensive and potentially use a lot of memory. I'd say they should be optional, but in the case of indirect ints I think not having them at all is better.

The FlatBuffers JS API also doesn't have any check for wrong offsets, it is a very high cost to pay to try and make this foolproof.

Yeah agree, this is also way the deduplication strategy is set do false per default. To be honest I don't thin that many users will use a builder in JS, I assume >90% users will just call encode with a dict or array.

js/flexbuffers.js

aardappel · 2020-06-18T16:44:07Z

@krojew do you mind giving this a quick look?

krojew · 2020-06-18T17:02:53Z

I would very much like to see it implemented in TS, rather than JS. This way we would have safer code, TS implementation covered and can compile to JS.

js/flexbuffers.js

RReverser · 2020-06-18T17:09:54Z

js/flexbuffers.js

+    const byteWidth = 1 << (packedType & 3);
+    const valueType = packedType >> 2;
+    let length = -1;
+    return {


This would be better done as a proper class to avoid constructing new objects and closures on each construction.

js/flexbuffers.js

RReverser · 2020-06-18T17:13:38Z

js/flexbuffers.js

+    return offsetStackValue(vecOffset, flexbuffers.ValueType.VECTOR, bitWidth);
+  }
+
+  function StackValue(type, width, value, _offset) {


Same as in other places, prefer real classes instead.

aardappel · 2020-06-18T18:14:50Z

js/flexbuffers.js

+flexbuffers.BitWidth.width = (value) => {
+  if (Number.isInteger(value)) {
+    const v = value < 0 ? value * -1 : value;
+    if (v >> 7 === 0) return flexbuffers.BitWidth.WIDTH8;


Comments from others in my team (not me, I have no idea):
"That's just incorrect, e.g. for v = 0x100000001 which is not an 8-bit integer but would take the return here."

C++ version:

flatbuffers/include/flatbuffers/flexbuffers.h

Lines 183 to 193 in 9abb2ec

inline BitWidth WidthU(uint64_t u) {

#define FLATBUFFERS_GET_FIELD_BIT_WIDTH(value, width) \

{ \

if (!((u) & ~((1ULL << (width)) - 1ULL))) return BIT_WIDTH_##width; \

}

FLATBUFFERS_GET_FIELD_BIT_WIDTH(u, 8);

FLATBUFFERS_GET_FIELD_BIT_WIDTH(u, 16);

FLATBUFFERS_GET_FIELD_BIT_WIDTH(u, 32);

#undef FLATBUFFERS_GET_FIELD_BIT_WIDTH

return BIT_WIDTH_64;

}

not sure why I felt a macro was necessary.. apologies :)

Well Number type is really broken in JavaScript. The C++ way you described, does not work either ;).

Good news, there is BigInt type, which works in a sane way. Bad news it is currently not supported by all browsers. Most notable Safari, on desktop and on iOS. But the version of macOS and iOS which are introduced yesterday and will be rolled out this fall include Safari which supports BigInt. Generally we need BigInt to to build a buffer, reading will work fine until we need to access a value which is stored as an 8 byte int / uint. If some one have a great idea for fixing it. You have my attention.

Bigint is also going to be slow, as it will be implemented as a dynamically allocated object in many cases.

I have no idea how to solve these kinds of problems in JS. From what I hear, to do bit-twiddling, you can basically rely on that working for 32-bit integers.. any bigger integers and you'd have to split it up manually. That's what FlatBuffers does for 64-bit numbers.

I removed BigInt / BigUInt conversions when checking for int / unit width and when reading int / uint it is used only if supported by the platform.

mzaks · 2020-06-19T12:50:00Z

I would very much like to see it implemented in TS, rather than JS. This way we would have safer code, TS implementation covered and can compile to JS.

I was thinking about TypeScript as well. But the request in #5949 was explicitly for JavaScript. And the infrastructure code for FlatBuffers is also written in JavaScript, even though there is an option in flatc to generate TypeScript explicitly.

So bottom line, I guess I could port the JS code to TS. But only if people agree that it is really worth it.

krojew · 2020-06-19T12:54:50Z

So bottom line, I guess I could port the JS code to TS. But only if people agree that it is really worth it.

Given you get better maintainability with safer code and cover two implementations at once - I would say it's worth it :)

…and ValueTypeUtil accordingly. Removing defensive checks. Introducing fix for large int numbers by converting them to BigInt type. Introducing handling for BigInt type in `add` method. Using TextEncoder and Decoder to handle string / utf8 conversion.

…of keys off while building FlexBuffer. Implements quick sort and choses quick sort if the number of keys is bigger then 20. Removes unnecessary dict lookups in BitWidthUtil helper functions

… usage

mzaks · 2020-08-18T15:52:18Z

Sorry for the radio silence.

I did a bit of refactoring to address some of the feedback.

I don't think that I will take the time and write a TypeScript port, given that the user facing API is very slim.
Most of the users will use:

let buffer = flexbuffers.encode(value); // converts a JS object into Uint8Array
let o = flexbuffers.toObject(buffer.buffer); // converts an `ArrayBufferLike` representing a FlexBuffer into JS object

There was also a suggestion to use JS classes and not just objects and functions.
I would gladly help refactoring the current implementation, but I don't see myself doing it.
I am not convinced that it will be a huge performance benefit as the objects are created once and switching to classes will be rather a cosmetic change.
The implementation is also fairly unit tested so there is no big risk in refactoring.

Also for a real performance critical scenarios like creation of buffer on Node.js I would rather recommend to have a WebAssembly (Rust, C++ bridge). I see this implementation as a simple single file drop in solution.

mzaks · 2020-08-24T11:18:30Z

@aardappel could you please point me to issues which need more attention from me.

aardappel · 2020-08-24T19:02:51Z

@mzaks well, I don't really know what "good JS" looks like, so that is why I am hoping you've taken into account what the above reviewers say, who know much better than me.

As far as the actual code structure is concerned, I would want it to match the C++ implementation as closely as possible, as I know that has a) gone through a lot of testing, and b) encodes the intended semantics of the serialization format and API as closely as possible. But I cannot go line by line and compare this PR with the C++ code, that is something you'll have to do. If you're confident you have done that, we can merge, if there's not any other reviewers.

Maybe @CasperN @paulovap @dmitriykovalev who all worked on FlexBuffers implementations recently want to have a quick look.

mzaks · 2020-08-25T08:09:59Z

@mzaks well, I don't really know what "good JS" looks like, so that is why I am hoping you've taken into account what the above reviewers say, who know much better than me.

As far as the actual code structure is concerned, I would want it to match the C++ implementation as closely as possible, as I know that has a) gone through a lot of testing, and b) encodes the intended semantics of the serialization format and API as closely as possible. But I cannot go line by line and compare this PR with the C++ code, that is something you'll have to do. If you're confident you have done that, we can merge, if there's not any other reviewers.

Maybe @CasperN @paulovap @dmitriykovalev who all worked on FlexBuffers implementations recently want to have a quick look.

@aardappel I am confident that this port is functional and is interoperable with C++ produced binary, based on the unit tests I wrote. One of them is also based on the golden FlexBuffer, which was produced during Rust FlexBuffer development. In regards to "good JS" I have to be honest and say that JS was never my "focus" language. I use it for over a decade but very sporadically. So there are probably thing which can be improved upon and I am happy to review the changes, but IMHO current implementation and unit tests are a good functioning starting point.

As I mentioned in a previous comment, for a heavy use case I would anyways recommend to have a WebAssembly version, which can be derived from C++, Rust, or I could write a Lobster implementation just for fun of it 😀.

aardappel · 2020-08-27T23:15:50Z

or I could write a Lobster implementation just for fun of it 😀.

I've definitely thought of that, but since Lobster already incorporates some of the C++ FlatBuffers code, it be most attractive to make it use the C++ FlexBuffer code as well.

normano · 2020-08-30T00:46:03Z

@aardappel Seems to me you don't have any issues with the code but still blocking it? Why not invite some of the others as reviewers that you mentioned to look into it so they can put block instead?

aardappel · 2020-08-31T19:15:52Z

@normano I am not blocking anything. I already invited them, and was simply waiting to see if any would comment. I guess they don't, so lets merge this as-is.

* Adding FlexBuffers support for Dart language * Introduce snapshot method. * Fix docu * Replacing extension methods with static methods in order to support older Dart version * Improving code based on PR feedback. Mainly rename refactoring. * Addressing all PR feedback which does not need clarification * exchange dynamic type with Object * Adds better API documentation. [] operator throws a very descriptive exception in case of a bad key. * Implementation of JavaScript FlexBuffers decoder * implements JS FlexBuffers builder * replacing _toF32 with Math.fround * Introducing test for BigInt number * Moving functions from BitWitdth & ValueType object into BitWidthUtil and ValueTypeUtil accordingly. Removing defensive checks. Introducing fix for large int numbers by converting them to BigInt type. Introducing handling for BigInt type in `add` method. Using TextEncoder and Decoder to handle string / utf8 conversion. * rename variable * Lets user turn deduplication strategies for strings, keys and vector of keys off while building FlexBuffer. Implements quick sort and choses quick sort if the number of keys is bigger then 20. Removes unnecessary dict lookups in BitWidthUtil helper functions * make iwidth and uwidth computation simpler and faster * Making redInt and readUint a bit faster and shim the BigInt / BigUint usage

mzaks added 11 commits April 10, 2020 14:34

Adding FlexBuffers support for Dart language

2c8fe37

Introduce snapshot method.

13145dd

Fix docu

ca6ef99

Replacing extension methods with static methods in order to support o…

02f6f9c

…lder Dart version

Improving code based on PR feedback. Mainly rename refactoring.

30db013

Addressing all PR feedback which does not need clarification

49c5e5e

exchange dynamic type with Object

781f0aa

Adds better API documentation.

5f3a4a6

[] operator throws a very descriptive exception in case of a bad key.

Merge branch 'master' of github.com:google/flatbuffers

4ae84cf

Implementation of JavaScript FlexBuffers decoder

e462a70

implements JS FlexBuffers builder

d76436c

mzaks mentioned this pull request Jun 16, 2020

[JavaScript] Flexbuffers support #5949

Closed

aardappel reviewed Jun 18, 2020

View reviewed changes

js/flexbuffers.js Outdated Show resolved Hide resolved

aardappel requested changes Jun 18, 2020

View reviewed changes

RReverser reviewed Jun 18, 2020

View reviewed changes

js/flexbuffers.js Show resolved Hide resolved

RReverser reviewed Jun 18, 2020

View reviewed changes

js/flexbuffers.js Show resolved Hide resolved

RReverser reviewed Jun 18, 2020

View reviewed changes

aardappel reviewed Jun 18, 2020

View reviewed changes

mzaks added 7 commits June 23, 2020 09:40

replacing _toF32 with Math.fround

c087425

Introducing test for BigInt number

6554c43

rename variable

b1f46c2

Lets user turn deduplication strategies for strings, keys and vector …

39973e5

…of keys off while building FlexBuffer. Implements quick sort and choses quick sort if the number of keys is bigger then 20. Removes unnecessary dict lookups in BitWidthUtil helper functions

make iwidth and uwidth computation simpler and faster

cbe5412

Making redInt and readUint a bit faster and shim the BigInt / BigUint…

4a639cc

… usage

aardappel approved these changes Aug 31, 2020

View reviewed changes

aardappel merged commit 71aca81 into google:master Aug 31, 2020

bjornharrtell mentioned this pull request Sep 9, 2020

[JS/TS] Modernize TypeScript / JavaScript flatbuffers support #6095

Merged

3 tasks

	inline BitWidth WidthU(uint64_t u) {
	#define FLATBUFFERS_GET_FIELD_BIT_WIDTH(value, width) \
	{ \
	if (!((u) & ~((1ULL << (width)) - 1ULL))) return BIT_WIDTH_##width; \
	}
	FLATBUFFERS_GET_FIELD_BIT_WIDTH(u, 8);
	FLATBUFFERS_GET_FIELD_BIT_WIDTH(u, 16);
	FLATBUFFERS_GET_FIELD_BIT_WIDTH(u, 32);
	#undef FLATBUFFERS_GET_FIELD_BIT_WIDTH
	return BIT_WIDTH_64;
	}

[JS] FlexBuffers Support #5973

[JS] FlexBuffers Support #5973

Uh oh!

Conversation

mzaks commented Jun 16, 2020

Uh oh!

Uh oh!

aardappel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aardappel commented Jun 18, 2020

Uh oh!

krojew commented Jun 18, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzaks commented Jun 19, 2020

Uh oh!

krojew commented Jun 19, 2020

Uh oh!

mzaks commented Aug 18, 2020

Uh oh!

mzaks commented Aug 24, 2020

Uh oh!

aardappel commented Aug 24, 2020

Uh oh!

mzaks commented Aug 25, 2020

Uh oh!

aardappel commented Aug 27, 2020

Uh oh!

normano commented Aug 30, 2020

Uh oh!

aardappel commented Aug 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone