Improve the efficiency of writing snapshots

[gibson042]

closes: #6225
refs: #6188

Description

• Use TMPDIR for temporary files.
• Parallelize reading raw snapshot data for hash computation and compression.

Security Considerations

n/a

Documentation Considerations

We might want to mention somewhere that snapshot creation puts some temporary files in the system temporary directory (as communicated by e.g. the POSIX TMPDIR environment variable).

Testing Considerations

Unit tests verify stable functionality, but the performance changes theirselves should be evaluated on a real network.

[gibson042]

I'll be resolving these "hoist" TODOs while the PR goes through CI, along with updating now-redundant definitions for e.g. path vs. stream parameters. They were defined inline during development, and left in place for now to avoid delaying the PR (which can be merged with them depending upon urgency).

[mhofman]

@gibson042 I just pushed the changes we paired on

[gibson042]

@mhofman Some further cleanup.

[gibson042]

Would withTempName be simpler on top of aggregateTryFinally?

  async function withTempName(fn, prefix = 'tmp') {
    const name = await ptmpName({
      tmpdir: root,
      template: `${prefix}-XXXXXX.xss`,
    });
    return aggregateTryFinally(
      () => fn(name),
      // Ignore file deletion errors.
      () => unlink(name).catch(sink),
    );
  }

Or maybe the point is moot, since load can be refactored to eliminate withTempName.

[mhofman]

I suppose we can use the same tmpFile approach for load.

[gibson042]

Cleanup ignores return values and the other uses of cleanup.push don't suppress them like this.

Suggested change

	cleanup.push(() => {
	snapReader.destroy();
	});
	cleanup.push(() => snapReader.destroy());

[mhofman]

The return value would be adopted in the promise result in the finally stage, meaning the stream object would be kept around for longer than necessary.

[gibson042]

But just in the time between running the function in PromiseAllOrErrors and throwing away the aggregated value, which should be inconsequential.

[mhofman]

Thinking about this, we can't use tmpFile here since it's not this process that opens the file, but not worth another CI churn to change the comment.

[gibson042]

The refactoring I have in mind would be updating the signature of saveRaw to accept a file handle or stream rather than a file name, but I understand that may not even be possible.

[mhofman]

Oh yeah we decompress to the file, then tell xsnap the filename, so it'd work.

[mhofman]

Thinking about this more, shouldn't we wait for previous steps to be done to continue with the next cleanup step? Disposal of resource is hard!

[mhofman]

Not an actual problem since only the last cleanup (first queued) is actually async. Let's follow-up to clean this up though.

[mhofman]

For future reference, internally the os.tmpdir() call is used, which currently has the following implementation in node:

    path = safeGetenv('TMPDIR') ||
           safeGetenv('TMP') ||
           safeGetenv('TEMP') ||
           '/tmp';