Replace buffer implementation with standard Uint8Array

[jfirebaugh]

The conditional use of node's Buffer versus a shim based on Uint8Array leads to subtle incompatibilities between node and the browser. I just spent a couple hours debugging an issue in mapbox-gl-js that stems from the following difference:

var Pbf = require('pbf');
var pbf = new Pbf();
pbf.writeVarint(0);
console.log(pbf.finish().buffer.byteLength);

In the browser, this prints 16, as expected. In node, it prints 8192.

To eliminate this gotcha, pbf should use standard Uint8Array's everywhere, and drop usage of Buffer.

[jfirebaugh]

Upstream issue: nodejs/node#6744

[mourner]

I think the main reason to use Buffer instead of Uint8Array was that native write, toString and other shimmed methods were simply much faster in the Node internal version. This may have changed with new Node versions — we need to check this again.

[mourner]

Yeah, just confirmed by running the benchmark in Node on both:

# Node Buffer
decode vector tile with pbf x 750 ops/sec ±2.59% (84 runs sampled)
encode vector tile with pbf x 596 ops/sec ±1.70% (80 runs sampled)

# Uint8Array shim
decode vector tile with pbf x 404 ops/sec ±1.17% (83 runs sampled)
encode vector tile with pbf x 631 ops/sec ±1.96% (84 runs sampled)

Writing is surprisingly slightly faster, but reading performance drop is dramatic (almost twice as slow) — we can't allow such a regression.

[jfirebaugh]

It's on the writing side that I lost most of a day to debugging a subtle difference between Buffer and Uint8Array, namely that slice() returns a new Uint8Array but a view of the underlying Buffer.

Can we use Uint8Array unconditionally for writing, or at least ensure that finish() returns a fresh Uint8Array in both implementations?

[mourner]

I don't yet see a good solution here. We can't switch the shim for writing only because you create a Pbf object once and can then mix read/write on it. And converting from Buffer to Uint8Array in finish would require a full copy of the data.

Would it be acceptable to leave the current implementation but add a warning to the finish method description in the docs? E.g. warning about type difference and advocating the use of pbf.length instead of result.buffer.byteLength.

[jfirebaugh]

Nobody will find that comment until they've already spent a long time debugging some obscure issue.

You've already benchmarked that using Uint8Array for writes is actually faster. So why not use them?

[jfirebaugh]

Workaround in gl-js: mapbox/mapbox-gl-js@57be2ce

[mourner]

You've already benchmarked that using Uint8Array for writes is actually faster. So why not use them?

As I mentioned earlier, we can only switch to Uint8Array for both read/write at the same time, and making reads 2 times slower is not an option. I'm investigating why exactly it's slower, but can't promise to find an actionable cause.

[jfirebaugh]

I still think we should just use and accept the performance of Uint8Array. High-performance node applications are going to want to use a native module for pbfs anyway.

The current code forces copying the data in the browser when you do new Pbf(uint8Array).

[mourner]

Working on this. Managed to get the Uint8Array decoding perf drop to ~9% from ~46%.