Fix loadgen and runner to accommodate endo bundles #32

mhofman · 2021-10-16T04:21:21Z

The introduction of endo zip bundles in Agoric/agoric-sdk#3273 required that contracts be NESM and bloated up the size of the slog file (probably in addition to other changes to the slog output). This caused the runner to both slow to a crawl when processing the slog output of the chain node, and made it impossible to deploy the loadgen code.

This PR fixes the slog processing by implementing a new line splitter stream transform that keeps the data as a Buffer, but outputs in object mode so that buffers are not re-concatenated. This dropped the parsing of the bootstrap slog from 3 minutes to mere seconds. As a result, a couple timing adjustments could be made:

Only print an event delay if its meaningful (somewhat arbitrarily set at more than 100ms)
Cancel timeout / sleep callbacks to avoid a temporarily hung exiting process if the timeout is raced and ignored

To accommodate the new endo bundle format while keeping backwards compatibility with older SDKs, split the deployed contract code in its own package with type: module. That field is ignored by the old bundler and satisfies both the new bundler and RESM that is still used by the deploy script (for backwards compatibility as well).

Once these issues were fixed, I've validated that the loadgen is compatible with most recent agoric-sdk, including with Agoric/agoric-sdk#3736.

As usual, best reviewed commit by commit.

warner

looks pretty good to me, although I'll leave the Buffer parsing parts to more experienced eyes

mhofman · 2021-10-30T00:51:37Z

@kriskowal could I request your eyes on the NESM stuff, and maybe the Buffer parsing?

runner/lib/helpers/buffer-line-transform.d.ts

kriskowal · 2021-10-30T15:14:27Z

runner/lib/helpers/buffer-line-transform.js

+    /** @type {number} */
+    let breakLength;
+    if (!breakValue || typeof breakValue === 'number') {
+      breakLength = 1;
+    } else if (Buffer.isBuffer(breakValue)) {
+      breakLength = breakValue.length;
+    } else {
+      breakLength = Buffer.from(breakValue, breakEncoding).length;
+    }
+    this._breakLength = breakLength;


It would probably be simpler to normalize the break to a single type, or even defer that responsibility to the user.

Deferring is exactly what this transform does, as it passes through to buf.indexOf(). However I do need to advance the search by the number of bytes of what had been found, which is what this calculates.

runner/lib/helpers/buffer-line-transform.js

kriskowal · 2021-10-30T15:18:26Z

runner/lib/helpers/buffer-line-transform.js

+            const concatLength = this._chunks.reduce(
+              (acc, { length }) => acc + length,
+              endOffset,
+            );


I can read this. A utility function might be nicer. A for loop might be kinder on both people and performance.

I don't like reduce much either, but gotta admit that accumulation of a value is basically what it's originally meant for.

kriskowal · 2021-10-30T15:19:18Z

runner/lib/helpers/buffer-line-transform.js

+              Buffer.concat(
+                [...this._chunks.splice(0, this._chunks.length), buf],
+                concatLength,
+              ),


Buffer concatenation is expensive and avoidable.

Unfortunately I didn't find a way to avoid it here. The stream is in block mode to allow the consumer to get a single object representing the line split at the configured break value. Node doesn't seem to have a way to create a buffer for which the underlying data is a list of buffers. Since the consumer expects a Buffer-like object, I didn't want to build such a wrapper. When I saw the performance was good enough for my use case, I left it at concat. If you know of a better way that doesn't require a ton of engineering, I'm open to suggestions.

FYI I did find https://www.npmjs.com/package/buffers, but that basically does a concat when producing a slice out, so that's not any better than this.

Does calling this._writeItem on each chunk not work? I would think that would avoid an intermediate allocation.

By default node streams may concatenate bytes as it sees fit. They also have an optional object mode, which treats the data in an opaque manner (even if the data is a Buffer of bytes).

In this case the goal of the transform is to produce a sequence of lines in the form of a Buffer of bytes, and consumers expect to handle a line as a single Buffer object. As such the readable portion of the stream transform is configured in object mode.

Some of the handling in the consumer involves pipping into another stream and converting either the whole line or a subset to a string. While I could probably build a "Buffers" object that kept a list of the original buffers and supported those operations somehow, I figured one intermediate allocation wouldn't be the end of the world.

kriskowal · 2021-10-30T15:20:44Z

runner/lib/helpers/buffer-line-transform.js

+   * The BufferLineTransform is reading String or Buffer content from a Readable stream
+   * and writing each line as a Buffer in object mode


Some optimizations for both human and machine readers are more obvious if you pick either string or bytes and not generalize both.

Node streams are usually built to handle both string and buffer, so since it was literally a single line to accept a string, I supported it.

A noteworthy design flaw of Node.js 😉

runner/lib/helpers/buffer-line-transform.js

kriskowal · 2021-10-30T15:25:03Z

runner/lib/helpers/buffer-line-transform.js

+        buf = buf.subarray(endOffset);
+      }
+      cb();
+    } catch (err) {


This probably catches more error classes than intended.

Such as? Do you see any problem with passing it all through the stream error handling logic?

I’m sure it’s fine in its form, but:

Node.js deopts any function that contains try/catch.

This broad of a catch clause will also defer programming errors (e.g., ReferenceError) to the stream handler. This is approximately analogous to what happens with promises regardless, so only something to avoid if it’s easy.

This broad of a catch obscures what would throw “invalid data type” specifically.

Yeah this definitely assumes there are no programming errors. But I have to somehow guard against bad input, and since the whole function does input processing, I'm not sure the try/catch could be much more targeted.

mhofman requested review from warner and kriskowal October 16, 2021 04:21

mhofman added 5 commits October 16, 2021 04:24

fix(runner): stream slog data as Buffer until parsing event

68ac266

fix(runner): cleanup sleep timeout to prevent hanging process

64b4f6c

feat(runner): Only print delayed events

08e7c7c

fix(loadgen): Make contract compatible with NESM endo bundles

e26ccb5

feat(loadgen): restore faucet task

e6c1259

mhofman force-pushed the mhofman/fix-endo-bundling branch from 4b8a802 to e6c1259 Compare October 16, 2021 04:25

warner approved these changes Oct 18, 2021

View reviewed changes

kriskowal approved these changes Oct 30, 2021

View reviewed changes

kriskowal reviewed Oct 30, 2021

View reviewed changes

fix(runner): buffer-line-transform: handle multi-byte search values

0a0a480

mhofman requested a review from kriskowal October 30, 2021 22:23

kriskowal approved these changes Oct 31, 2021

View reviewed changes

mhofman merged commit 2f1f43e into main Oct 31, 2021

mhofman deleted the mhofman/fix-endo-bundling branch January 17, 2022 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix loadgen and runner to accommodate endo bundles #32

Fix loadgen and runner to accommodate endo bundles #32

mhofman commented Oct 16, 2021

warner left a comment •

edited

Loading

mhofman commented Oct 30, 2021

kriskowal Oct 30, 2021

mhofman Oct 30, 2021

kriskowal Oct 30, 2021

mhofman Oct 30, 2021

kriskowal Oct 30, 2021

mhofman Oct 30, 2021

mhofman Oct 30, 2021

kriskowal Oct 31, 2021

mhofman Oct 31, 2021

kriskowal Oct 30, 2021

mhofman Oct 30, 2021

kriskowal Oct 31, 2021

kriskowal Oct 30, 2021

mhofman Oct 30, 2021

kriskowal Oct 31, 2021

mhofman Oct 31, 2021

		* The BufferLineTransform is reading String or Buffer content from a Readable stream
		* and writing each line as a Buffer in object mode

Fix loadgen and runner to accommodate endo bundles #32

Fix loadgen and runner to accommodate endo bundles #32

Conversation

mhofman commented Oct 16, 2021

warner left a comment • edited Loading

Choose a reason for hiding this comment

mhofman commented Oct 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warner left a comment •

edited

Loading