Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SnapshotCreator API #281
base: master
Are you sure you want to change the base?
Add SnapshotCreator API #281
Changes from 6 commits
6b4e736
b6360e2
44297f8
88b05d0
b66880e
fbf1281
ebdfa43
acaa5fb
4030029
ef9e910
eec9dcd
ebb5fa8
3a6bb0f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this field could be removed, since the slice holds the data size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth panicking here trying to call a function on a snapshot creator thats likely been disposed of (either through
Dispose
or from callingCreate
to make the blob)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets stick with the error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thes.ptr == nil
and!s.defaultContextAdded
cases could result in a panic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems worth mentioning that this also disposes of the isolate associated with it