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

Add a powerPreference option #2084

Merged
merged 1 commit into from Jan 5, 2023

Conversation

greggman
Copy link
Contributor

Not sure how to add this here's an attempt.

You can pass powerPreference=low-power or
power-preference=high-performance in the query string.

Issue: #2083


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

@github-actions
Copy link

Previews, as seen when this build job started (ed9cec8):
Run tests | View tsdoc

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

LGTM, see comment about possible alternative

if (defaultRequestAdapterOptions) {
// eslint-disable-next-line @typescript-eslint/unbound-method
const oldFn = impl.requestAdapter;
impl.requestAdapter = function (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally I'd avoid interposing on the API since it means tests aren't running against the implementation itself, but it's probably fine since it only happens when you explicitly enable it. There are some tests which this will have slightly surprising effects on though, specifically device_allocation.spec.ts and requestAdapter.spec.ts.

The other option would be to change the requestAdapter call in device_pool.ts, which would affect most tests but not the few that call requestAdapter themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I looked into adding it to device_pool.ts and it wasn't clear to me how to make it work. First I tried adding a requestAdapterOptions to DevicePool and that needed to be passed down to DescriptorToHolderMap and then to DeviceHolder. Then I tried pass in a default at the top where it creates the device pool singleton. But, you obviously can't have a function to set the defualt in that file since the function needs to be called before the singleton is created.

I put the functions to set (and get) the default options in a separate file alongsite device_pool.ts but when I tried to call it from standalone.ts I got an error calling into that part of the tree is off-limits.

Maybe there's an easier way but for now I decided to leave it as is. I'm happy to refactor into device_pool.ts with some guidance on how to do it.

@greggman greggman force-pushed the specify-power-preference branch 2 times, most recently from 4d486cf to 55cea45 Compare January 5, 2023 19:20
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

Previews, as seen when this build job started (55cea45):
Run tests | View tsdoc

Not sure how to add this here's an attempt.

You can pass `powerPreference=low-power` or
`power-preference=high-performance` in the query string.
@greggman greggman enabled auto-merge (rebase) January 5, 2023 20:02
@greggman greggman merged commit d7f4a9e into gpuweb:main Jan 5, 2023
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

Previews, as seen when this build job started (f282e43):
Run tests | View tsdoc

@greggman greggman deleted the specify-power-preference branch February 14, 2023 20:38
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.

None yet

2 participants