Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

[io] Fix a bug in concatenateTypedArrays #1181

Merged
merged 4 commits into from
Jul 23, 2018
Merged

Conversation

caisq
Copy link
Collaborator

@caisq caisq commented Jul 23, 2018

Previously, it was incorrectly assumed that the buffer of a
TypedArray has the same byteLength as the TypedArray itself.
The two can actually be different if the TypedArray is created
from an offset and a length from an ArrayBuffer, e.g.,
const x = new Float32Array(myBuffer, 32, 4).
This actually happens during the desrialization of a tf.Model.

This PR fixes it by checking whether the byteLength and
buffer.byteLength of a TypedArray are equal, and if not, creating
a new TypedArray with the buffer that matches the actual byteLength.

BUG


This change is Reviewable

Previously, it was incorrectly assumed that the `buffer` of a
`TypedArray` has the same `byteLength` as the `TypedArray` itself.
The two can actually be different if the `TypedArray` is created
from an offset and a length from an `ArrayBuffer`, e.g.,
`const x = new Float32Array(myBuffer, 32, 4)`.
This actually happens during the desrialization of a `tf.Model`.

This PR fixes it by checking whether the `byteLength` and
`buffer.byteLength` of a `TypedArray` are equal, and if not, creating
a new `TypedArray` with the buffer that matches the actual `byteLength`.

BUG
@caisq caisq requested review from dsmilkov and nsthorat July 23, 2018 18:01
Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq, @nsthorat, and @dsmilkov)


src/io/io_utils.ts, line 135 at r1 (raw file):

        x.byteLength === x.buffer.byteLength ? x :
                                               new (x.constructor as any)(x));
    if (!(x as any instanceof Float32Array || x as any instanceof Int32Array ||

I think you can you get rid of the any's here by using the following a few lines above:

```xs.forEach((x: TypedArray) => {`




<!-- Sent from Reviewable.io -->

@caisq caisq merged commit 80c2f71 into tensorflow:master Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants