Skip to content
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

feat: Support custom host objects in ValueSerializer #1322

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

lrowe
Copy link
Contributor

@lrowe lrowe commented Sep 16, 2023

Add v8::ValueSerializerImpl::{has_custom_host_object,is_host_object} equivalents for v8::ValueSerializer::Delegate::{HasCustomHostObject,IsCustomHostObject}.

This enables serializing custom host objects without embedder fields.

@CLAassistant
Copy link

CLAassistant commented Sep 16, 2023

CLA assistant check
All committers have signed the CLA.

@lrowe
Copy link
Contributor Author

lrowe commented Sep 16, 2023

Note. While I am fairly confident the changes in this PR are correct, I am still working through the corresponding changes in deno_core required to make use of this functionality.

lrowe added a commit to lrowe/deno_core that referenced this pull request Sep 16, 2023
This avoids substantial slowdowns caused by host objects using embedded fields.
Depends on: denoland/rusty_v8#1322

Current status:
For some reason v8::Symbol::for_global is returning a different symbol to the one defined in 01_core.js.

running 2 tests from ./cli/tests/unit/message_channel_test.ts
messagechannel ... ok (34ms)
messagechannel clone port ...result=Some(false) constructor=Object keys.length=0, identity hash:499026937
result=Some(false) constructor=MessagePort keys.length=4, identity hash:499026937
0 symbol "Deno.core.customHostObject" is false, hash 253732785
1 symbol "[[webidl.brand]]" is false, hash 764160415
2 symbol "undefined" is false, hash 758685447
3 symbol "id" is false, hash 997007839
lrowe added a commit to lrowe/deno_core that referenced this pull request Sep 16, 2023
This avoids substantial slowdowns caused by host objects using embedded fields.
Depends on: denoland/rusty_v8#1322

Current status:
For some reason v8::Symbol::for_global is returning a different symbol to the one defined in 01_core.js.

```
$ ./target/debug/deno test cli/tests/unit/message_channel_test.ts
running 2 tests from ./cli/tests/unit/message_channel_test.ts
messagechannel ... ok (35ms)
messagechannel clone port ...result=Some(false) constructor=Object keys.length=0, identity hash:560437950 is_object false is_symbol true
result=Some(false) constructor=MessagePort keys.length=4, identity hash:560437950 is_object false is_symbol true
0 symbol "Deno.core.customHostObject" is false, hash 253732785 is_object false is_symbol true
1 symbol "[[webidl.brand]]" is false, hash 764160415 is_object false is_symbol true
2 symbol "undefined" is false, hash 758685447 is_object false is_symbol true
3 symbol "id" is false, hash 997007839 is_object false is_symbol true

Uncaught error from ./cli/tests/unit/message_channel_test.ts FAILED
messagechannel clone port ... cancelled (0ms)
```
lrowe added a commit to lrowe/deno_core that referenced this pull request Sep 17, 2023
Avoids substantial slowdown caused by host objects with embedder fields.

* Depends on: denoland/rusty_v8#1322
* Depends on: denoland/rusty_v8#1324
* Deno bug: denoland/deno#12067
lrowe added a commit to lrowe/deno_core that referenced this pull request Sep 17, 2023
Avoids substantial slowdown caused by host objects with embedder fields.

* Depends on: denoland/rusty_v8#1322
* Depends on: denoland/rusty_v8#1324
* Deno bug: denoland/deno#12067
lrowe added a commit to lrowe/deno_core that referenced this pull request Sep 17, 2023
Avoids substantial slowdown caused by host objects with embedder fields.

* Depends on: denoland/rusty_v8#1322
* Depends on: denoland/rusty_v8#1324
* Deno bug: denoland/deno#12067
@lrowe lrowe force-pushed the lrowe-custom-host-object branch 3 times, most recently from 86c185a to f3f55bf Compare September 18, 2023 22:48
lrowe added a commit to lrowe/deno_core that referenced this pull request Sep 27, 2023
Avoids substantial slowdown caused by host objects with embedder fields.

* Depends on: denoland/rusty_v8#1322
* Depends on: denoland/rusty_v8#1324
* Deno bug: denoland/deno#12067
lrowe added a commit to lrowe/deno_core that referenced this pull request Sep 27, 2023
src/value_serializer.rs Outdated Show resolved Hide resolved
lrowe added a commit to lrowe/deno_core that referenced this pull request Nov 7, 2023
Avoids substantial slowdown caused by host objects with embedder fields.

* Depends on: denoland/rusty_v8#1322
* Depends on: denoland/rusty_v8#1324
* Deno bug: denoland/deno#12067
@lrowe
Copy link
Contributor Author

lrowe commented Nov 17, 2023

With the change to has_custom_host_object I think this is ready for merging. @legendecas could you approve your review unless you have other concerns?

Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @bartlomieju can you also take a look?

@bartlomieju
Copy link
Member

Yup, I'll review tonight.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too, no comments. Thank you @lrowe!

@bartlomieju bartlomieju merged commit ec2c901 into denoland:main Nov 20, 2023
8 checks passed
lrowe added a commit to lrowe/deno_core that referenced this pull request Nov 27, 2023
Avoids substantial slowdown caused by host objects with embedder fields.

* Depends on: denoland/rusty_v8#1322
* Depends on: denoland/rusty_v8#1324
* Deno bug: denoland/deno#12067
lrowe added a commit to lrowe/deno_core that referenced this pull request Nov 28, 2023
Avoids substantial slowdown caused by host objects with embedder fields.

* Depends on: denoland/rusty_v8#1322
* Depends on: denoland/rusty_v8#1324
* Deno bug: denoland/deno#12067
lrowe added a commit to lrowe/deno_core that referenced this pull request Nov 28, 2023
Avoids substantial slowdown caused by host objects with embedder fields.

* Depends on: denoland/rusty_v8#1322
* Depends on: denoland/rusty_v8#1324
* Deno bug: denoland/deno#12067
lrowe added a commit to lrowe/deno_core that referenced this pull request Nov 28, 2023
Avoids substantial slowdown caused by host objects with embedder fields.

* Depends on: denoland/rusty_v8#1322
* Depends on: denoland/rusty_v8#1324
* Deno bug: denoland/deno#12067
lrowe added a commit to lrowe/deno_core that referenced this pull request Nov 28, 2023
Avoids substantial slowdown caused by host objects with embedder fields.

* Depends on: denoland/rusty_v8#1322
* Depends on: denoland/rusty_v8#1324
* Deno bug: denoland/deno#12067
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants