Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JavaScript] Flexbuffers support #5949

Closed
loganzartman opened this issue Jun 6, 2020 · 14 comments
Closed

[JavaScript] Flexbuffers support #5949

loganzartman opened this issue Jun 6, 2020 · 14 comments
Labels

Comments

@loganzartman
Copy link

Are there any plans or existing projects working on support for Flexbuffers in JavaScript?
One of my projects would really benefit from a schema-less option, and the C++ implementation for Flexbuffers is very pleasant to use.

@aardappel
Copy link
Collaborator

I don't know of anyone working on it.. and yes it be great fit for JS (or any dynamically typed language, really).

Maybe one of the implementors of the other ports has interest (@paulovap @mzaks @CasperN)

@mzaks
Copy link
Contributor

mzaks commented Jun 8, 2020

I was thinking about doing it anyways. So I can take it.

@mzaks
Copy link
Contributor

mzaks commented Jun 10, 2020

@aardappel I am done with FlexBuffers decoder. Should I create a PR already, so it is not that much code to review, or should I write an encoder as well in single PR?

@aardappel
Copy link
Collaborator

@mzaks I'd say lets do a code review once you think it is ready to merge.

@mzaks
Copy link
Contributor

mzaks commented Jun 16, 2020

I just created a PR: #5973

@lgeiger
Copy link

lgeiger commented Sep 7, 2020

@mzaks @aardappel Thanks for the PR and review!

I tried out the code for and for me it seems like toObject() doesn't always work correctly. Consider the following test case with flexbuffer data generated from C++:

// json data: {'channels_in': 64, 'dilation_height_factor': 1, 'dilation_width_factor': 1, 'fused_activation_function': 1, 'pad_values': 1, 'padding': 0, 'stride_height': 1, 'stride_width': 1}
var data = [99, 104, 97, 110, 110, 101, 108, 115, 95, 105, 110, 0, 100, 105, 108, 97, 116, 105, 111, 110, 95, 104, 101, 105, 103, 104, 116, 95, 102, 97, 99, 116, 111, 114, 0, 100, 105, 108, 97, 116, 105, 111, 110, 95, 119, 105, 100, 116, 104, 95, 102, 97, 99, 116, 111, 114, 0, 102, 117, 115, 101, 100, 95, 97, 99, 116, 105, 118, 97, 116, 105, 111, 110, 95, 102, 117, 110, 99, 116, 105, 111, 110, 0, 112, 97, 100, 95, 118, 97, 108, 117, 101, 115, 0, 112, 97, 100, 100, 105, 110, 103, 0, 115, 116, 114, 105, 100, 101, 95, 104, 101, 105, 103, 104, 116, 0, 115, 116, 114, 105, 100, 101, 95, 119, 105, 100, 116, 104, 0, 8, 130, 119, 97, 76, 51, 41, 34, 21, 8, 1, 8, 64, 1, 1, 1, 1, 0, 1, 1, 4, 4, 4, 4, 4, 4, 4, 4, 16, 36, 1];
flexbuffers.toObject(new Uint8Array(data).buffer);

Which fails with:

/Users/lgeiger/code/flatbuffers/js/flexbuffers.js:257
    while (dataView.getUint8(keyIndirectOffset + length) !== 0) {
                    ^
RangeError: Offset is outside the bounds of the DataView
    at DataView.getUint8 (<anonymous>)
    at keyForIndex (/Users/lgeiger/code/flatbuffers/js/flexbuffers.js:257:21)
    at Object.toObject (/Users/lgeiger/code/flatbuffers/js/flexbuffers.js:403:23)
    at Object.flexbuffers.toObject (/Users/lgeiger/code/flatbuffers/js/flexbuffers.js:435:42)

Do you have an idea what's going on there? Let me know if I you would prefer me to open a new issue.

@mzaks
Copy link
Contributor

mzaks commented Sep 7, 2020

@mzaks @aardappel Thanks for the PR and review!

I tried out the code for and for me it seems like toObject() doesn't always work correctly. Consider the following test case with flexbuffer data generated from C++:

// json data: {'channels_in': 64, 'dilation_height_factor': 1, 'dilation_width_factor': 1, 'fused_activation_function': 1, 'pad_values': 1, 'padding': 0, 'stride_height': 1, 'stride_width': 1}
var data = [99, 104, 97, 110, 110, 101, 108, 115, 95, 105, 110, 0, 100, 105, 108, 97, 116, 105, 111, 110, 95, 104, 101, 105, 103, 104, 116, 95, 102, 97, 99, 116, 111, 114, 0, 100, 105, 108, 97, 116, 105, 111, 110, 95, 119, 105, 100, 116, 104, 95, 102, 97, 99, 116, 111, 114, 0, 102, 117, 115, 101, 100, 95, 97, 99, 116, 105, 118, 97, 116, 105, 111, 110, 95, 102, 117, 110, 99, 116, 105, 111, 110, 0, 112, 97, 100, 95, 118, 97, 108, 117, 101, 115, 0, 112, 97, 100, 100, 105, 110, 103, 0, 115, 116, 114, 105, 100, 101, 95, 104, 101, 105, 103, 104, 116, 0, 115, 116, 114, 105, 100, 101, 95, 119, 105, 100, 116, 104, 0, 8, 130, 119, 97, 76, 51, 41, 34, 21, 8, 1, 8, 64, 1, 1, 1, 1, 0, 1, 1, 4, 4, 4, 4, 4, 4, 4, 4, 16, 36, 1];
flexbuffers.toObject(new Uint8Array(data).buffer);

Which fails with:

/Users/lgeiger/code/flatbuffers/js/flexbuffers.js:257
    while (dataView.getUint8(keyIndirectOffset + length) !== 0) {
                    ^
RangeError: Offset is outside the bounds of the DataView
    at DataView.getUint8 (<anonymous>)
    at keyForIndex (/Users/lgeiger/code/flatbuffers/js/flexbuffers.js:257:21)
    at Object.toObject (/Users/lgeiger/code/flatbuffers/js/flexbuffers.js:403:23)
    at Object.flexbuffers.toObject (/Users/lgeiger/code/flatbuffers/js/flexbuffers.js:435:42)

Do you have an idea what's going on there? Let me know if I you would prefer me to open a new issue.

Hi Lukas, what did you use to build the buffer?

@mzaks
Copy link
Contributor

mzaks commented Sep 7, 2020

@lgeiger Ah sorry you just replied it was C++. Then let me check a bit further.

@mzaks
Copy link
Contributor

mzaks commented Sep 7, 2020

So the buffer you got with C++ is 10 bytes shorter then the one I get if encode the same object with JS. I need a bit more time to get to the bottom of it and today is already a bit late. I will get back at you tomorrow. @lgeiger

@lgeiger
Copy link

lgeiger commented Sep 7, 2020

@mzaks Thanks for taking a look, I'll try to investigate further

@lgeiger
Copy link

lgeiger commented Sep 7, 2020

I double checked, and if I generate the flexbuffer with Python I get 160 bytes:

fbb = flexbuffers.Builder()
with fbb.Map():
    fbb.Int("channels_in", 64)
    fbb.Int("dilation_height_factor", 1)
    fbb.Int("dilation_width_factor", 1)
    fbb.Int("fused_activation_function", 1)
    fbb.Int("pad_values", 1)
    fbb.Int("padding", 0)
    fbb.Int("stride_height", 1)
    fbb.Int("stride_width", 1)
data = fbb.Finish()
list(data)

#  [99, 104, 97, 110, 110, 101, 108, 115, 95, 105, 110, 0, 100, 105, 108, 97, 116, 105, 111, 110, 95, 104, 101, 105, 103, 104, 116, 95, 102, 97, 99, 116, 111, 114, 0, 100, 105, 108, 97, 116, 105, 111, 110, 95, 119, 105, 100, 116, 104, 95, 102, 97, 99, 116, 111, 114, 0, 102, 117, 115, 101, 100, 95, 97, 99, 116, 105, 118, 97, 116, 105, 111, 110, 95, 102, 117, 110, 99, 116, 105, 111, 110, 0, 112, 97, 100, 95, 118, 97, 108, 117, 101, 115, 0, 112, 97, 100, 100, 105, 110, 103, 0, 115, 116, 114, 105, 100, 101, 95, 104, 101, 105, 103, 104, 116, 0, 115, 116, 114, 105, 100, 101, 95, 119, 105, 100, 116, 104, 0, 8, 130, 119, 97, 76, 51, 41, 34, 21, 8, 1, 8, 64, 1, 1, 1, 1, 0, 1, 1, 4, 4, 4, 4, 4, 4, 4, 4, 16, 36, 1]

and 170 bytes if I convert it with JavaScript using:

flexbuffers.encode({
      channels_in: 64,
      dilation_height_factor: 1,
      dilation_width_factor: 1,
      fused_activation_function: 1,
      pad_values: 1,
      padding: 0,
      stride_height: 1,
      stride_width: 1,
    })

// [99, 104, 97, 110, 110, 101, 108, 115, 95, 105, 110, 0, 100, 105, 108, 97, 116, 105, 111, 110, 95, 104, 101, 105, 103, 104, 116, 95, 102, 97, 99, 116, 111, 114, 0, 100, 105, 108, 97, 116, 105, 111, 110, 95, 119, 105, 100, 116, 104, 95, 102, 97, 99, 116, 111, 114, 0, 102, 117, 115, 101, 100, 95, 97, 99, 116, 105, 118, 97, 116, 105, 111, 110, 95, 102, 117, 110, 99, 116, 105, 111, 110, 0, 112, 97, 100, 95, 118, 97, 108, 117, 101, 115, 0, 112, 97, 100, 100, 105, 110, 103, 0, 115, 116, 114, 105, 100, 101, 95, 104, 101, 105, 103, 104, 116, 0, 115, 116, 114, 105, 100, 101, 95, 119, 105, 100, 116, 104, 0, 0, 8, 0, 132, 0, 122, 0, 101, 0, 81, 0, 57, 0, 48, 0, 42, 0, 30, 0, 16, 2, 8, 64, 1, 1, 1, 1, 0, 1, 1, 4, 4, 4, 4, 4, 4, 4, 4, 16, 36, 1]

@lutzroeder
Copy link

@mzaks indirect() should subtract uint instead of int.

function indirect(dataView, offset, width) {
    const step = readUInt(dataView, offset, width);
    return offset - step;
}

@mzaks
Copy link
Contributor

mzaks commented Sep 8, 2020

So @lutzroeder is correct and the bug surfaced a few more issues with this implementation. I fixed it all and submitted a PR: #6107

As mentioned in PR comment the buffers created with previous implementation are compatible with C++ and other correct implementations of FlexBuffers. They are just a bit bigger in size, because values which can fit in 1 byte as uint (127 < x < 255 ) were stored as 2 bytes. This is where the 10 bytes difference comes from, which I found in my first investigation.

@github-actions
Copy link

github-actions bot commented Mar 9, 2021

This issue is stale because it has been open 6 months with no activity. Please comment or this will be closed in 14 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants