src: register external references for source code

[kvakil]

Currently we use external strings for internalized builtin source code. However when a snapshot is taken, any external string whose resource is not registered is flattened into a SeqString (see ref). The result is that any module source code stored in the snapshot does not use external strings after deserialization. This patch registers an external string resource for each internalized builtin's source. The savings are substantial: ~1.9 MB of heap memory per isolate, or ~44% of an otherwise empty isolate's heap usage:

$ node --expose-gc -p 'gc(),process.memoryUsage().heapUsed'
4190968
$ ./node --expose-gc -p 'gc(),process.memoryUsage().heapUsed'
2327536

The savings can be even higher for user snapshots which may include more internal modules.

Doing this with the existing UnionBytes abstraction was tricky, because UnionBytes only creates an external string resource when ToStringChecked is called. We change UnionBytes to no longer own the data, and shift ownership of the data (in the case where the data is not static) to a new external resource class. We can then register any resource whose data is static, since we know those are precisely the internalized builtins.

Refs: https://github.com/v8/v8/blob/d2c8fbe9ccd1a6ce5591bb7dd319c3c00d6bf489/src/snapshot/serializer.cc#L633

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/startup

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50341/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50355/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50365/

[kvakil]

OK, PTAL. The big change is that we now only have one map by moving the complexity into UnionBytes following @joyeecheung's advice. UnionBytes has a resource_ which does the actual "owning". The registration is done by a new generated RegisterExternalReferencesForInternalizedBuiltinCode, so we don't need to do any runtime branching on whether the resources in the sources_ map are internalized or externalized builtins.

[joyeecheung]

Looks much nicer, thanks, a couple of remaining suggestions, but over all looks good

[kvakil]

Thanks for the comments! They should all be addressed now.

[joyeecheung]

LGTM, thanks

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50392/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50406/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51110/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51719/

[kvakil]

Can someone please re-run the failing Jenkins build for me? I want to get this merged in, it's been languishing for a while

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51737/

[legendecas]

Sorry for the delay. Adding to my watch list to land.

[nodejs-github-bot]

Landed in 0736d0b

[mmomtchev]

@kvakil Are you sure that v8::String::ExternalStringResourceBase doesn't expect to own the allocation? I have another PR that is affected by this and in my case v8::String::ExternalStringResourceBase::Dispose calls free on the received string when destroying the isolate?

[kvakil]

@mmomtchev this is V8's documentation/default implementation of v8::String::ExternalStringResourceBase::Dispose:

/**
 * Internally V8 will call this Dispose method when the external string
 * resource is no longer needed. The default implementation will use the
 * delete operator. This method can be overridden in subclasses to
 * control how allocated external string resources are disposed.
 */
virtual void Dispose() { delete this; }

Since this diff has StaticExternalByteResource override Dispose to be empty, we never execute delete this:

void Dispose() override {
  // We ignore Dispose calls from V8, even if we "own" a resource via
  // owning_ptr_. All instantiations of this class are static or owned by a
  // static map, and will be destructed when static variables are destructed.
}

so I believe this is correct.