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: update v8 flags #707

Merged
merged 5 commits into from
Apr 22, 2024
Merged

Conversation

petamoriken
Copy link
Contributor

@petamoriken petamoriken commented Apr 21, 2024

related: denoland/deno#17944 (comment), denoland/deno#23450

Removed flags enabled by default and added flags for Explicit Resource Management and Float16Array.

" --harmony-temporal",
" --js-explicit-resource-management",
Copy link
Member

Choose a reason for hiding this comment

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

Oh nice, last time I checked (last week) it still wasn't supported. Is it just partial support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. It appears that test262 is still insufficient and I'm not sure how widely it has been covered.

Copy link
Contributor

Choose a reason for hiding this comment

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

V8 12.4 can parse using declarations but does not actually dispose the resources. The functionality was first added in V8 12.5.79.

SuppressedError() crashes the process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info! I've removed this flag.

core/runtime/setup.rs Show resolved Hide resolved
@@ -23,13 +23,11 @@ fn v8_init(

let base_flags = concat!(
" --wasm-test-streaming",
" --harmony-import-assertions",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we should disable this one now - it will start crashing on assert keyword. Or do you plan to conditionally enable it in Deno?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assert keyword is also enabled by default, so this change has no effect.

https://github.com/v8/v8/blob/12.4.254.12/src/flags/flag-definitions.h#L295

I plan to conditionally disable it in Deno. It seems that it is probably enabled if both --harmony-import-assertions and --no-harmony-import-assertions are specified.

core/runtime/setup.rs Show resolved Hide resolved
@petamoriken
Copy link
Contributor Author

It failed to install libc6-dbg by apt-get in CI

The following NEW packages will be installed:
  libc6-dbg valgrind
0 upgraded, 2 newly installed, 0 to remove and 20 not upgraded.
Need to get 27.9 MB of archives.
After this operation, 98.0 MB of additional disk space will be used.
Get:1 file:/etc/apt/apt-mirrors.txt Mirrorlist [142 B]
Ign:2 http://azure.archive.ubuntu.com/ubuntu jammy-updates/main amd64 libc6-dbg amd64 2.35-0ubuntu3.6
Get:3 http://azure.archive.ubuntu.com/ubuntu jammy/main amd64 valgrind amd64 1:3.18.1-1ubuntu2 [14.1 MB]
Ign:2 http://archive.ubuntu.com/ubuntu jammy-updates/main amd64 libc6-dbg amd64 2.35-0ubuntu3.6
Ign:2 http://security.ubuntu.com/ubuntu jammy-updates/main amd64 libc6-dbg amd64 2.35-0ubuntu3.6
Err:2 mirror+file:/etc/apt/apt-mirrors.txt jammy-updates/main amd64 libc6-dbg amd64 2.35-0ubuntu3.6
  404  Not Found [IP: 52.147.219.192 80]
E: Failed to fetch mirror+file:/etc/apt/apt-mirrors.txt/pool/main/g/glibc/libc6-dbg_2.35-0ubuntu3.6_amd64.deb  404  Not Found [IP: 52.147.219.192 80]

@bartlomieju bartlomieju enabled auto-merge (squash) April 22, 2024 19:03
@bartlomieju bartlomieju merged commit a0f1b57 into denoland:main Apr 22, 2024
18 checks passed
@petamoriken petamoriken deleted the update/v8-flags branch April 22, 2024 19:46
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