Implement a bundle reader for Web #3097

wu-hui · 2020-05-21T16:18:44Z

The implementation is based around Web's ReadableStream. This does not work on NodeJS unfortunately. I need to fix that, but want to send this out before it gets too big.

google-oss-bot · 2020-05-21T16:26:51Z

Binary Size Report

Affected SDKs

No changes between base commit (f347dea) and head commit (7b6fa7a).

Test Logs

Base (f347deaf): https://github.com/firebase/firebase-js-sdk/actions/runs/111678381
Head (7b6fa7af): https://github.com/firebase/firebase-js-sdk/actions/runs/125438703

wu-hui

Thanks for the comments!

I replied some of them without making any code changes, because I feel we should address those before I actually change anything.

wu-hui · 2020-05-28T15:17:49Z

packages/firestore/src/util/bundle.ts

+    // Update the internal buffer to drop the read length and json string.
+    this.buffer = this.buffer.slice(lengthBuffer.length + length);
+
+    if (!this.metadata) {


Metadata being the first element of a stream is a feature, IMO. I cannot think of a use case where metadata being in the middle of a stream is useful, and I think it's better to reject bundles without metadata upfront, than try to cope with it.

I am willing to add it if someone actually find this useful, and request for that.

wu-hui · 2020-05-28T15:21:16Z

packages/firestore/src/util/bundle.ts

+  // If reached end of the stream, returns a null.
+  private async readLength(): Promise<Uint8Array | null> {
+    let position = this.indexOfOpenBracket();
+    while (position < 0) {


Do you mean we always encode length in a fixed length string, like instead of 123, we always encode it as 00000123?

This might be easier to implement (and less error-prone), but note when dealing with ReadableStream we cannot control how much bytes we read back. So we still need to pull more data from the stream if it just happens that our first read is not sufficient.

wu-hui · 2020-05-28T15:51:43Z

packages/firestore/src/util/bundle.ts

+      );
+      newBuffer.set(this.buffer);
+      newBuffer.set(result.value, this.buffer.length);
+      this.buffer = newBuffer;


Note we cannot control how many bytes we read from ReadableStream, so an internal buffer cannot be avoided completely.

Currently implementation only pulls more data when necessary, so the internal buffer is already kept as small as possible when there is a need to pull and concat.

Actually, I don't think it's O(n^2) copies in reality, because most of the data read from underlying stream is copied only once (into existing internal buffer), the part that will be copied again is the part that is the start of an element, which will be copied again when the next pull is called.

The worst case is when one element requires multiple pulls, and each pulls will have to copy the old and new buffer. I feel this might not be very common though.

wu-hui · 2020-05-28T15:53:52Z

packages/firestore/test/unit/util/bundle.test.ts

+
+function genericBundleReadingTests(bytesPerRead: number): void {
+  // Setting up test data.
+  const meta = {


It definitely makes it shorter, but might not be as more readable as the size reduction? Because readers now have to read the built bundle content to make sure the tests are doing the right thing?

schmidt-sebastian · 2020-06-02T16:51:16Z

packages/firestore/src/util/bundle_reader.ts

+ * Create a `ReadableStream` from a underlying buffer.
+ *
+ * @param data: Underlying buffer.
+ * @param bytesPerRead: How many bytes to read from the underlying buffer from each read through the stream.


Nit: Line wrap

schmidt-sebastian · 2020-06-02T16:54:19Z

packages/firestore/src/util/bundle_reader.ts

+ */
+export class BundleReader {
+  // Cached bundle metadata.
+  private metadata?: BundleMetadata | null;


All comments that describe methods, classes or members should be JSDoc. Comments about code logic are prefixed by //.

The question you should ask yourself: Should this comment show up in code completion or API documentation? If so, it should be JSDoc /** */. See here:

firebase-js-sdk/packages/firestore/src/local/indexeddb_persistence.ts

Line 213 in ef5fb1b

/** Our 'visibilitychange' listener if registered. */

schmidt-sebastian · 2020-06-02T17:17:18Z

packages/firestore/src/util/bundle_reader.ts

+      await this.nextElement();
+    }
+
+    return this.metadata!;


I still think this code is a bit surprising. It is not clear to me why nextElement() sets this.metadata.

I played around with your PR a bit. What do you think of these changes? https://paste.googleplex.com/5782882595373056

It looks nice!

One thing I added there is the introduction of an async queue, it looks calls to nextElement() might interleave with each other without it.

schmidt-sebastian · 2020-06-02T17:17:46Z

packages/firestore/src/util/bundle_reader.ts

+      }
+    }
+
+    return element;


The changes linked above simplify this and the related method above.

schmidt-sebastian · 2020-06-02T17:19:25Z

packages/firestore/src/util/bundle_reader.ts

+      }
+    }
+
+    return this.buffer.slice(0, position);


Should this method advance the buffer as well and return the length as a number, rather than returning the encoded byte array (this is not part of my change)?

The length buffer also counts in bytes processed, we need that information for SizedBundleElement, that is why i choose to do it this way.

Alternative is to return a number (length) and another one (bytes consumed in bundles), which is also not ideal.

Applied buffer advancement here.

schmidt-sebastian · 2020-06-02T17:19:55Z

packages/firestore/src/util/bundle_reader.ts

+  // number of bytes, pulling more data from the underlying stream if needed.
+  //
+  // Returns a string decoded from the read bytes.
+  private async readJsonString(start: number, length: number): Promise<string> {


schmidt-sebastian · 2020-06-02T17:21:15Z

packages/firestore/src/util/bundle_reader.ts

+      }
+    }
+
+    return this.textDecoder.decode(this.buffer.slice(start, start + length));


Similarly, could we advance the buffer here? It might make the code simpler as start would always be zero now.

schmidt-sebastian · 2020-06-02T17:24:18Z

packages/firestore/test/unit/util/bundle.test.ts

+  };
+  const metaString = lengthPrefixedString(meta);
+
+  const doc1Meta = {


One benefit of not using JSON is that you could encore types here. It might help if you assigned a strict type here:

const doc1Meta : BundleMetadata = {}

Your test is currently mostly untyped. If you change the schema, your test will still compile and may even pass depending on if you update the test expectations. By enforcing types manually, you can help us to diagnose problems in the future.

Done. Also fixed an error in api proto d.ts.

schmidt-sebastian · 2020-06-02T17:27:15Z

packages/firestore/test/unit/util/bundle.test.ts

+    'throws with ill-formatted bundle with bytesPerRead ' + bytesPerRead,
+    async () => {
+      await expect(
+        parseThroughBundle('metadata: "no length prefix"', bytesPerRead)


In the Web SDK, we generally assert on the actual error messages as this allows us to verify that the messages make sense. Do you mind doing this here? You can look at validation.test.ts for examples.

schmidt-sebastian · 2020-06-02T17:27:28Z

packages/firestore/src/util/bundle_reader.ts

+
+  // Pulls more data from underlying stream to internal buffer.
+  // Returns a boolean indicating whether the stream is finished.
+  private async pullMoreDataToBuffer(): Promise<number> {


schmidt-sebastian · 2020-06-02T17:34:08Z

packages/firestore/src/util/bundle_reader.ts

+  }
+
+  // Pulls more data from underlying stream to internal buffer.
+  // Returns a boolean indicating whether the stream is finished.


If you keep this as is, please update the method comment to stay that it returns the length read. I did notice however that this information is not used, so we could keep the boolean.

Changed back to boolean.

schmidt-sebastian

This is super close. Thanks for the updates!

schmidt-sebastian · 2020-06-04T04:49:32Z

packages/firestore/src/util/bundle_reader.ts

+          const payload = (element || { payload: null }).payload;
+          this.metadata.reject(
+            new Error(`The first element of the bundle is not a metadata, it is
+          ${JSON.stringify(payload)}`)


Suggested change

const payload = (element || { payload: null }).payload;

this.metadata.reject(

new Error(`The first element of the bundle is not a metadata, it is

${JSON.stringify(payload)}`)

this.metadata.reject(

new Error(`The first element of the bundle is not a metadata element, it is:

${JSON.stringify(element?.payload)}`)

This leads to compiler error, actually.

It works for me.

Hmm, my intellij keeps complaining, but compile from CLI works. I guess there is some config i need to fix for my intellij.

schmidt-sebastian · 2020-06-04T04:50:19Z

packages/firestore/src/util/bundle_reader.ts

+      error => {
+        this.metadata.reject(error);
+      }


Suggested change

error => {

this.metadata.reject(error);

}

error => this.metadata.reject(error)

Removes two lines.

schmidt-sebastian · 2020-06-04T04:51:33Z

packages/firestore/src/util/bundle_reader.ts

+  async nextElement(): Promise<SizedBundleElement | null> {
+    // Ensures `nextElementImpl` calls are executed sequentially before they
+    // modifies internal buffer.
+    return this.asyncQueue.enqueue(() => this.nextElementImpl());


We should not read user data on the async queue. All data that is processed on the async queue needs to be verified first. For bundles, that means that the parsing should happen outside.

This is an async queue specifically for this particular reader, it does not affect the queue that most of the sdk is using.

If we hit an error reading the bundle, we will abort the rest anyways, so maybe here it is fine?

As discussed offline, the queue has been removed. This improves the error handling for the user as we can distinguish between different error messages and only raise the AsyncQueue errors for internal failures.

schmidt-sebastian · 2020-06-04T04:56:44Z

packages/firestore/src/util/bundle_reader.ts

+    let position = this.indexOfOpenBracket();
+    while (position < 0) {
+      const done = await this.pullMoreDataToBuffer();
+      if (done) {
+        if (this.buffer.length === 0) {
+          return null;
+        }
+        position = this.indexOfOpenBracket();
+        // Underlying stream is closed, and we still cannot find a '{'.
+        if (position < 0) {
+          this.raiseError(
+            'Reached the end of bundle when a length string is expected.'
+          );
+        }
+      } else {
+        position = this.indexOfOpenBracket();
+      }
+    }


Can we find a way to cut down on the size of this block? For example, we shouldn't invoke indexOfOpenBracket() thrice.

Something like:

let position: number; while ((position = this.indexOfOpenBracket()) == -1) { const done = await this.pullMoreDataToBuffer(); if (done) { break; } } if (position < 0) { this.raiseError('....'); }

Thanks for the hint!

schmidt-sebastian · 2020-06-04T04:57:51Z

packages/firestore/test/unit/util/bundle.test.ts

+  };
+  const limitToLastQueryString = lengthPrefixedString(limitToLastQuery);
+
+  async function getAllElement(


s/getAllElement/getAllElements

schmidt-sebastian · 2020-06-04T05:00:52Z

packages/firestore/test/unit/util/bundle.test.ts

+    );
+  }
+
+  async function parseThroughBundle(


This reads like "passthrough" but it's not. Maybe generateBundleAndParse?

schmidt-sebastian · 2020-06-04T05:02:53Z

packages/firestore/src/util/bundle_reader.ts

+    }
+    this.reader = (this.bundleStream as ReadableStream).getReader();
+
+    this.nextElement().then(


Maybe add a small inline comment before this line:

// Read the metadata (which is the first element)

schmidt-sebastian

Thanks for all the updates!

schmidt-sebastian · 2020-06-04T19:23:30Z

packages/firestore/src/util/bundle_reader.ts

+      return null;
+    }
+
+    position = this.indexOfOpenBracket();


You can drop position from the loop above and assign it here as a const.

schmidt-sebastian · 2020-06-04T19:26:49Z

packages/firestore/src/util/bundle_reader.ts

+          const payload = (element || { payload: null }).payload;
+          this.metadata.reject(
+            new Error(`The first element of the bundle is not a metadata, it is
+          ${JSON.stringify(payload)}`)


It works for me.

wu-hui changed the base branch from wuandy/Bundles to wuandy/SchemaChangeForBundles May 21, 2020 16:49

wu-hui added 3 commits May 21, 2020 13:49

Renaming interfaces without leading I

9602712

Initial commit of bundle reading - for web only.

c5e783e

Tests only run when it is not Node.

5e7fb89

wu-hui force-pushed the wuandy/BundleReader branch from a99583f to 5e7fb89 Compare May 21, 2020 17:50

wu-hui requested review from alikn, avolkovi, davideast, egilmorez, Feiyang1, hiranya911, hsubox76, jamesdaniels, sam-gc, scottcrossen, yuchenshi, zijianjoy and zwu52 as code owners May 21, 2020 17:50

wu-hui changed the base branch from wuandy/SchemaChangeForBundles to wuandy/Bundles May 21, 2020 17:50

Fix redundant imports

1ee1615

wu-hui requested review from schmidt-sebastian and removed request for jamesdaniels, avolkovi, sam-gc, hiranya911, Feiyang1, davideast, egilmorez and alikn May 21, 2020 17:52

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian May 26, 2020

wu-hui commented May 28, 2020

View reviewed changes

wu-hui assigned schmidt-sebastian May 28, 2020

schmidt-sebastian removed their assignment May 28, 2020

Remove generator.

aa455bf

wu-hui assigned schmidt-sebastian and unassigned wu-hui May 29, 2020

schmidt-sebastian reviewed Jun 2, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui Jun 2, 2020

schmidt-sebastian reviewed Jun 2, 2020

View reviewed changes

schmidt-sebastian removed their assignment Jun 2, 2020

Address comments

782e76f

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jun 3, 2020

schmidt-sebastian reviewed Jun 4, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jun 4, 2020

Address more feedback.

f5e4474

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jun 4, 2020

wu-hui added 3 commits June 4, 2020 14:27

Race condition.

c60f8dc

Wait for metadata to be read before move on to rest elements.

e005485

Remove only.

6d9d5cd

schmidt-sebastian approved these changes Jun 4, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jun 4, 2020

Fix payload printing.

a869466

wu-hui merged commit b0c8299 into wuandy/Bundles Jun 5, 2020

firebase locked and limited conversation to collaborators Jul 5, 2020

Implement a bundle reader for Web #3097

Implement a bundle reader for Web #3097

Uh oh!

Conversation

wu-hui commented May 21, 2020

Uh oh!

google-oss-bot commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

Test Logs

Uh oh!

wu-hui left a comment

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!

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!

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!

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!

schmidt-sebastian left a comment

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!

google-oss-bot commented May 21, 2020 •

edited

Loading