Add SnapshotCreator API #281

GustavoCaso · 2022-01-26T09:46:50Z

This PR allows users of v8go to use the existing API from V8 to create HeapSnapshots.

The v8go API is similar to the one provided by V8 and the SnapshotCreator class.

The API still need some work to have the full support that V8 provides, such as passing options to NewSnapshotCreator as well as allowing to store data arbitrary data on the snapshot AddData

In other to support passing the created snapshot_blob to the NewIsolate method, we had to allow passing options when creating a new isolate.

The old API:

v8go.NewIsolate()

The new API:

// data == *v8.StartupData
v8go.NewIsolate(v8go.WithStartupData(data))

Allowing to pass options to NewIsolate allows us to keep the existing API with breaking existing codebases 🎉

Creating a context from v8go now can be done using two methods:

The already existing one v8go.NewContext(iso)
The one introduced here v8go.NewContextFromSnapshot(iso, index)

The snapshot creator API

For every context that gets added to the snapshot creator, we need to make sure the Isolate is the same as the one associated with the snapshot creator class itself. That is the reason we have the GetIsolate() method, so we can create a new context using that one. This isolate is allocated by V8 when calling SnapshotCreator* creator = new SnapshotCreator;

When creating the snapshot blob we have two methods to add existing contexts to the snapshot creator:

SetDefaultContext -> This will set the context as the default one. From there every new context created from that isolate will have that context loaded, so calling v8go.NewContext(iso) will return a context with the default context loaded.
AddContext -> This will append to the list of additional contexts that a snapshot creator can have. The method returns an index, which we will be able to use to indicate to V8 when creating a new context from it. v8go.NewContextFromSnapshot(iso, index)

The V8 code that I used as a reference for creating the API is the one in the v8/test/cctest/test-serialize.cc file.

codecov · 2022-01-26T09:48:09Z

Codecov Report

Merging #281 (fbf1281) into master (db78170) will decrease coverage by 0.10%.
The diff coverage is 94.91%.

❗ Current head fbf1281 differs from pull request most recent head 3a6bb0f. Consider uploading reports for the commit 3a6bb0f to get more accurate results

@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
- Coverage   95.86%   95.76%   -0.11%     
==========================================
  Files          17       18       +1     
  Lines         580      637      +57     
==========================================
+ Hits          556      610      +54     
- Misses         15       17       +2     
- Partials        9       10       +1

Impacted Files	Coverage Δ
snapshot_creator.go	`91.42% <91.42%> (ø)`
context.go	`100.00% <100.00%> (ø)`
isolate.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db78170...3a6bb0f. Read the comment docs.

GustavoCaso · 2022-01-26T10:27:18Z

We could allow passing SnapshotCreatorOptions struct from the beginning even if we currently do nothing with it, that way in the future whenever we start adding support for the options on the snapshot creator class we would not break the existing API.

GustavoCaso · 2022-01-26T10:48:57Z

v8go.cc

+  Local<Context> local_ctx = ctx->ptr.Get(iso);
+  Context::Scope context_scope(local_ctx);
+
+  ContextFree(ctx);


The reason we have to free the internal context before adding it into the snapshot creator class is that the snapshot creator class doesn't know how to deserialize the internal values we store in the local context. Every time we allocate a new m_value and pushed to the context tracked_value(ctx, val) creates a reference that we have to remove before adding the context.

This could be made more consistent with V8 by just freeing the tracked values in SetDefaultContext or AddContext, along with mentioning this value invalidation in the documentation. It isn't ideal there are no use after free checks on values, but that was already the case (e.g. for Context.Close), so outside the scope of this PR.

v8go.cc

snapshot_creator.go

context.go

context_test.go

snapshot_creator.go

snapshot_creator_test.go

v8go.cc

Co-authored-by: Genevieve <genevieve.lesperance@shopify.com>

GustavoCaso · 2022-01-28T09:06:52Z

@rogchap If you have sometime I would appreciate your feedback on the new API for snapshot creation 😄

rogchap · 2022-01-28T10:29:22Z

On a very quick glance, the API remains very close to the CPP equivalent, and remains mostly backward compatible which is great.

I'll need to spend a little more time to look at this in detail.

Appreciate your contributions 💪

GustavoCaso · 2022-02-09T10:52:03Z

Hi @rogchap 👋

By any chance have you got some time to look into it?

context_test.go

isolate.go

snapshot_creator.go

genevieve · 2022-03-09T23:15:07Z

snapshot_creator.go

+// This Isolate must be use to create the contexts that later will be use to create the snapshot blob.
+func (s *SnapshotCreator) GetIsolate() (*Isolate, error) {
+	if s.ptr == nil {
+		return nil, errors.New("v8go: Cannot get Isolate after creating the blob")


Might be worth panicking here trying to call a function on a snapshot creator thats likely been disposed of (either through Dispose or from calling Create to make the blob)

It seems that the pattern among other functions #118 (comment)

Should we panic in all of the instances. It seems that it would make working with the Snapshot creator API much easier.

snapshotCreator := v8.NewSnapshotCreator() snapshotCreatorIso := snapshotCreator.GetIsolate() snapshotCreatorCtx := v8.NewContext(snapshotCreatorIso) defer snapshotCreatorCtx.Close() snapshotCreatorCtx.RunScript(`const add = (a, b) => a + b`, "add.js") snapshotCreatorCtx.RunScript(`function run() { return add(3, 4); }`, "main.js") snapshotCreator.SetDefaultContext(snapshotCreatorCtx) data := snapshotCreator.Create(v8.FunctionCodeHandlingClear) iso := v8.NewIsolate(v8.WithStartupData(data)) defer iso.Dispose() ctx, err := v8.NewContextFromSnapshot(iso, index) defer ctx.Close()

The only one that I'm not sure 100% we should panic is the one that errors when calling Create before adding a default context since this is something a developer can fix.

What do you think @genevieve?

Lets stick with the error

We should return errors when we expect the caller to handle them. The V8 API already does that, so we can normally keep the API similar to V8 in that respect, except that we use a panic to protect against crashes. The API will especially be annoying when getters like this one return an error.

snapshot_creator.go

snapshot_creator_test.go

v8go.cc

snapshot_creator.go

dylanahsmith · 2022-05-16T19:47:57Z

snapshot_creator.go

+	return &StartupData{data: data, raw_size: raw_size}, nil
+}
+
+// Dispose deletes the reference to the SnapshotCreator.


Seems worth mentioning that this also disposes of the isolate associated with it

dylanahsmith · 2022-05-16T19:51:26Z

snapshot_creator.go

+func (s *SnapshotCreator) Dispose() {
+	if s.ptr != nil {
+		C.DeleteSnapshotCreator(s.ptr)
+		s.ptr = nil


Seems like the isolate's internal pointer should be marked as nil (i.e. s.iso.ptr = nil), as is done in (*v8go.Isolate).Dispose(), to reflect the fact that it will be disposed by SnapshotCreator::~SnapshotCreator().

dylanahsmith · 2022-05-16T20:09:19Z

v8go.cc

+  SnapshotCreator* creator = new SnapshotCreator;
+  Isolate* iso = creator->GetIsolate();
+  rtn.creator = creator;
+  rtn.iso = iso;


This isolate differs from the one created from NewIsolate in that it doesn't have an internal context. I think that would likely result in a crash from using any API that tries to use that internal context, such as NewValue.

Can that internal context be created and just not be added to the snapshot?

dylanahsmith · 2022-05-16T20:38:09Z

snapshot_creator.go

+// StartupData stores the snapshot blob data
+type StartupData struct {
+	data     []byte
+	raw_size C.int


It seems like this field could be removed, since the slice holds the data size

dylanahsmith · 2022-05-16T22:04:19Z

v8go.cc

+  Local<Context> local_ctx = ctx->ptr.Get(iso);
+  Context::Scope context_scope(local_ctx);
+
+  ContextFree(ctx);


This could be made more consistent with V8 by just freeing the tracked values in SetDefaultContext or AddContext, along with mentioning this value invalidation in the documentation. It isn't ideal there are no use after free checks on values, but that was already the case (e.g. for Context.Close), so outside the scope of this PR.

dylanahsmith · 2022-05-16T22:30:29Z

snapshot_creator.go

+
+// StartupData stores the snapshot blob data
+type StartupData struct {
+	data     []byte


Don't we want to be able to persist or transfer the bytes so that the snapshot can be reused cross-process, similar to CompilerCachedData? To allow that, we need the bytes field to be exported so they can be used for IPC and so the struct can be constructed in another process.

Suggested change

data []byte

Bytes []byte

dylanahsmith · 2022-05-16T23:02:32Z

snapshot_creator.go

+// This Isolate must be use to create the contexts that later will be use to create the snapshot blob.
+func (s *SnapshotCreator) GetIsolate() (*Isolate, error) {
+	if s.ptr == nil {
+		return nil, errors.New("v8go: Cannot get Isolate after creating the blob")


We should return errors when we expect the caller to handle them. The V8 API already does that, so we can normally keep the API similar to V8 in that respect, except that we use a panic to protect against crashes. The API will especially be annoying when getters like this one return an error.

dylanahsmith · 2022-05-17T00:02:38Z

snapshot_creator.go

+	s.iso.ptr = nil
+
+	raw_size := rtn.raw_size
+	data := C.GoBytes(unsafe.Pointer(rtn.data), raw_size)


The v8::SnapshotCreator::CreateBlob documentation says it returns "{ nullptr, 0 } on failure". If this function is going to return an error, then I would definitely expect an error to be returned when V8 would return an error. Unfortunately, V8 is quite vague on what possible reasons there are for an error and the API doesn't expose an error message.

dylanahsmith · 2022-05-17T00:06:58Z

snapshot_creator.go

+
+// Create creates a snapshot data blob.
+// The snapshot creator instance is unsable after creating the snapshot data blob.
+func (s *SnapshotCreator) Create(functionCode FunctionCodeHandling) (*StartupData, error) {


StartupData is basically just a wrapper around a slice. It may as well be returned without the extra pointer indirection, since the error would indicate whether or not it is an error. Since V8 doesn't actually expose an error message, ok could be used for the error and the s.ptr == nil and !s.defaultContextAdded cases could result in a panic.

Danm87 · 2022-10-09T10:24:51Z

Hi,
this is something i'm interested in as well.
would appreciate to know what's the current blocker :)

GustavoCaso force-pushed the GustavoCaso/SnapshotCreator branch 3 times, most recently from 6ddb67c to 74b2e56 Compare January 26, 2022 10:24

GustavoCaso commented Jan 26, 2022

View reviewed changes

v8go.cc Show resolved Hide resolved

genevieve reviewed Jan 26, 2022

View reviewed changes

snapshot_creator.go Outdated Show resolved Hide resolved

genevieve reviewed Jan 26, 2022

View reviewed changes

snapshot_creator.go Show resolved Hide resolved

genevieve reviewed Jan 26, 2022

View reviewed changes

context.go Show resolved Hide resolved

genevieve reviewed Jan 26, 2022

View reviewed changes

context_test.go Outdated Show resolved Hide resolved

genevieve reviewed Jan 26, 2022

View reviewed changes

snapshot_creator.go Outdated Show resolved Hide resolved

genevieve reviewed Jan 26, 2022

View reviewed changes

snapshot_creator_test.go Outdated Show resolved Hide resolved

genevieve reviewed Jan 26, 2022

View reviewed changes

snapshot_creator_test.go Outdated Show resolved Hide resolved

genevieve reviewed Jan 26, 2022

View reviewed changes

v8go.cc Outdated Show resolved Hide resolved

genevieve reviewed Jan 26, 2022

View reviewed changes

v8go.cc Outdated Show resolved Hide resolved

Gustavo Caso and others added 5 commits January 28, 2022 09:54

Add SnapshotCreator API

6b4e736

Co-authored-by: Genevieve <genevieve.lesperance@shopify.com>

Improve error message for NewContextFromSnapshot

b6360e2

add documentation for the new API

44297f8

cleanup tests

88b05d0

remove unnecessary check for nullptr

b66880e

GustavoCaso force-pushed the GustavoCaso/SnapshotCreator branch from be2acf1 to cc6e686 Compare January 28, 2022 09:00

handle errors when calling NewContextFromSnapshot at V8 level

fbf1281

GustavoCaso force-pushed the GustavoCaso/SnapshotCreator branch from cc6e686 to fbf1281 Compare January 28, 2022 09:04