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

fix cts_runner #6840

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Conversation

turbocrime
Copy link

@turbocrime turbocrime commented Dec 31, 2024

Connections

#6839

plus two more issues, revealed after the above issue was fixed.

Description

cts_runner could not execute conformance tests due to broken deno state management.

this pr restores the functionality of cts_runner, by

  • fix panic in compute_pass and render_pass by using state.borrow::<super::Instance>() instead of state.borrow::<wgpu_core::global::Global>(), which is not present
  • fix panic in cts_runner main by providing Permissions struct to js_runtime state
  • fix test failure due to incorrect ser/de of ColorWrites

Testing

cts_runner can now be manually executed.

i've been using a script to execute tests, adapted from the deleted cts.yml workflow

cts_runner/cts.sh

#!/bin/sh

set -euo pipefail

if [[ "$PWD/cts_runner" != "$(dirname -- "$(readlink -f -- "$0")")" ]]; then
  echo "Run this script from the repository root."
  exit 1
fi

cd cts

if [[ "$(git rev-parse HEAD)" != "$(cat ../cts_runner/revision.txt)" ]]; then
  echo "CTS revision mismatch."
  cat ../cts_runner/revision.txt
  exit 2
fi

grep -v '^//' ../cts_runner/test.lst | while IFS=$' \t\r\n' read test; do
  echo "=== Running $test ==="
  cargo run --manifest-path ../cts_runner/Cargo.toml --frozen -- ./tools/run_deno --verbose "$test"
done

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@FrankenApps
Copy link
Contributor

Please fix your commits to only include the changed lines, otherwise this is impossible to review.

@turbocrime
Copy link
Author

Please fix your commits to only include the changed lines, otherwise this is impossible to review.

sorry for messy diff - still drafting pr. will mark as 'ready' soon

Comment on lines +158 to +160
state = |state| {
state.put(Permissions {});
}
Copy link
Author

Choose a reason for hiding this comment

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

permissions must be available in state before bootstrap. for cts_runner it's static, so i moved it here into the extension macro.

@@ -32,7 +32,7 @@ pub fn op_webgpu_compute_pass_set_pipeline(
.get::<WebGpuComputePass>(compute_pass_rid)?;

state
.borrow::<wgpu_core::global::Global>()
.borrow::<super::Instance>()
Copy link
Author

Choose a reason for hiding this comment

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

super::Instance wraps a wgpu_core::global::Global and is also actually present in the runtime state. it's used in locations that continued to pass tests after 77a83fb and using it consistently seems to correct the test panics.

Copy link
Author

Choose a reason for hiding this comment

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

several op functions in command_encoder, render_pass, and compute_pass use some gfx_ok and gfx_put macros.

i didn't add or remove any calls, but i noticed use seems to have become more inconsistent over time.

Comment on lines +4669 to +4680
/// Color write mask. Disabled color channels will not be written to.
///
/// Corresponds to [WebGPU `GPUColorWriteFlags`](
/// https://gpuweb.github.io/gpuweb/#typedefdef-gpucolorwriteflags).
#[repr(transparent)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "serde", serde(transparent))]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct ColorWrites(u32);

bitflags::bitflags! {
/// Color write mask. Disabled color channels will not be written to.
///
/// Corresponds to [WebGPU `GPUColorWriteFlags`](
/// https://gpuweb.github.io/gpuweb/#typedefdef-gpucolorwriteflags).
#[repr(transparent)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "serde", serde(transparent))]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct ColorWrites: u32 {
impl ColorWrites: u32 {
Copy link
Author

Choose a reason for hiding this comment

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

the bitflags macro and serde macros seem to be sensitive to ordering.

previously, the generated deserializer would expect a javascript string, but it's clearly a numeric type.

@turbocrime turbocrime marked this pull request as ready for review December 31, 2024 09:41
@turbocrime turbocrime requested review from crowlKats and a team as code owners December 31, 2024 09:41
@@ -195,14 +193,15 @@ pub fn op_webgpu_render_pass_execute_bundles(
#[serde]
pub fn op_webgpu_render_pass_end(
state: &mut OpState,
#[smi] _command_encoder_rid: ResourceId,
Copy link
Author

Choose a reason for hiding this comment

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

this parameter is unused in this function, but it is part of the denoland implementation of deno_webgpu, and it's expected to be present by 01_webgpu.js in this repository

wgpu/deno_webgpu/01_webgpu.js

Lines 3867 to 3870 in 61b7063

const { err } = op_webgpu_render_pass_end(
commandEncoderRid,
renderPassRid,
);

@@ -91,14 +91,15 @@ pub fn op_webgpu_compute_pass_dispatch_workgroups_indirect(
#[serde]
pub fn op_webgpu_compute_pass_end(
state: &mut OpState,
#[smi] _command_encoder_rid: ResourceId,
Copy link
Author

Choose a reason for hiding this comment

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

this parameter is unused in this function, but it is part of the denoland implementation of deno_webgpu, and it's expected to be present by 01_webgpu.js in this repository

wgpu/deno_webgpu/01_webgpu.js

Lines 4383 to 4386 in 61b7063

const { err } = op_webgpu_compute_pass_end(
commandEncoderRid,
computePassRid,
);

@turbocrime
Copy link
Author

uncertain how to add this to the changelog.

by intent, this is just fixing internal tools.

practically, i think the state management changes and the parameter changes probably fixed some real outwards issues, but i haven't looked for examples.

@cwfitzgerald
Copy link
Member

You can add it to the testing/internal category as we don't know of any direct problems this solves.

@cwfitzgerald
Copy link
Member

(Approved wgpu side)

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.

4 participants