-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[deno] Resolve CTS OOMs in CI #8930
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
base: trunk
Are you sure you want to change the base?
Conversation
Enable some tests that previously OOMed in CI.
| // Associate external memory with the device to encourage V8 to garbage | ||
| // collect devices promptly. | ||
| scope | ||
| .adjust_amount_of_external_allocated_memory(DEVICE_EXTERNAL_MEMORY_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.
Interesting, I wonder if we could do that in servo too.
ErichDonGubler
left a comment
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.
LGTM, though I'm concerned that I don't know enough about the Deno side to say that this is okay. I think it's easier to start with this good approximation that solves real problems, and then work towards refining this. So, my disposition is to merge first, ask @crowlKats if this is okay later…except that we may build a nontrivial amount of expected passes for tests that we might have to revert later.
I'll approve to unblock, and let other discussion decide how to proceed from here.
| webgpu:api,validation,capability_checks,limits,maxInterStageShaderVariables:createRenderPipeline,at_over:limitTest="atDefault";* | ||
| webgpu:api,validation,capability_checks,limits,maxInterStageShaderVariables:createRenderPipeline,at_over:limitTest="atMaximum";* | ||
| webgpu:api,validation,capability_checks,limits,maxInterStageShaderVariables:createRenderPipeline,at_over:limitTest="betweenDefaultAndMaximum";* |
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.
question: Is it feasible to simplify to …:createRenderPipeline,at_over:*?
|
|
||
| /// External memory associated with device and queue, to encourage V8 to garbage | ||
| /// collect devices promptly. This seems to be particularly important on DX12 | ||
| /// in CI, where any smaller power of two results in OOM errors. |
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.
nitpick: This is a crate sync'd between here and Deno upstream, so clarifying whose CI is failing here might be important.
|
I am forwarding this to someone else at Deno since I am unfamiliar with the memory related APIs. I'll let you know once I have a response. |
crowlKats
left a comment
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.
Its an all-ok from our side
Two changes to enable some CTS suites that would previously OOM in CI:
GPUQueueobject was lazily constructed the first time the queue was accessed from JS, if JS never accessed the queue at all, theGPUQueuewas never created, and thus never dropped, causing queue (and devices, since the queue has a reference to the device) to leak.cts_runner.Testing
Enables CTS tests that weren't working before, although it's not directed coverage for the changes.
Squash or Rebase? Rebase
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.