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

Use new V8 v2 write APIs #3464

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Use new V8 v2 write APIs #3464

wants to merge 1 commit into from

Conversation

danlapid
Copy link
Collaborator

@danlapid danlapid commented Feb 5, 2025

No description provided.

Copy link

github-actions bot commented Feb 5, 2025

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

@danlapid danlapid force-pushed the dlapid/use_new_v8_methods branch 7 times, most recently from ec34139 to 8e6d7e5 Compare February 5, 2025 14:20
@danlapid
Copy link
Collaborator Author

danlapid commented Feb 5, 2025

This seems to not really work.
The new APIs aren't really compatible with the old ones.
Two problems from what I can see:

  1. Missing the read_nchars variable means it's hard to tell how much of the original string was read.
  2. WriteOneByteV2 does DCHECK(IsOneByte()); which we fail in some of our tests that expect the value you be brokenly encoded successfully. We can patch that check probably
  3. WriteUtf8V2 works differently in some way I don't quite understand.

@anonrig anonrig self-assigned this Feb 5, 2025
@fhanau
Copy link
Collaborator

fhanau commented Feb 5, 2025

This seems to not really work. The new APIs aren't really compatible with the old ones. Two problems from what I can see:

  1. Missing the read_nchars variable means it's hard to tell how much of the original string was read.
  2. WriteOneByteV2 does DCHECK(IsOneByte()); which we fail in some of our tests that expect the value you be brokenly encoded successfully. We can patch that check probably
  3. WriteUtf8V2 works differently in some way I don't quite understand.

At least the 2) can be worked around well: WriteV2() is identical to WriteOneByteV2() except that it doesn't have the DCHECK that is causing us problems.
One difference in options based on the implementation of these in api.cc: NO_NULL_TERMINATION behavior may have changed – in the old mode we would add a null byte only if the input buffer was shorter than the output buffer. In the new mode we write even if they are the same size.

@anonrig
Copy link
Member

anonrig commented Feb 5, 2025

Upon talking with @danlapid, I'll take over this pull-request. I'll make some progress tomorrow.

@anonrig anonrig force-pushed the dlapid/use_new_v8_methods branch 2 times, most recently from 0ed5cc4 to e1f19fb Compare February 6, 2025 23:37
@anonrig anonrig force-pushed the dlapid/use_new_v8_methods branch from e1f19fb to 65f7ffc Compare February 7, 2025 14:54
@anonrig anonrig mentioned this pull request Feb 7, 2025
@anonrig anonrig force-pushed the dlapid/use_new_v8_methods branch from 65f7ffc to e7893d9 Compare February 11, 2025 16:01
@anonrig anonrig force-pushed the dlapid/use_new_v8_methods branch from e7893d9 to 30d797a Compare February 11, 2025 16:02
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.

3 participants