Skip to content

Add maxBufferSize limit #3366

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.

Sign up for GitHub

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

Jump to bottom
Merged
litherum merged 2 commits into from
Aug 25, 2022
Merged

Add maxBufferSize limit #3366

litherum merged 2 commits into from
Aug 25, 2022

Conversation

@litherum
Copy link
Contributor

@litherum litherum commented Aug 24, 2022

Fixes #1371.

@litherum litherum requested review from kainino0x and toji August 24, 2022 08:02
@github-actions
Copy link
Contributor

github-actions bot commented Aug 24, 2022

Previews, as seen when this build job started (b53a6f9):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

@github-actions
Copy link
Contributor

github-actions bot commented Aug 24, 2022

Previews, as seen when this build job started (a86d51d):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

@kainino0x kainino0x added the tacit resolution queue Editors have agreed and intend to land if no feedback is given label Aug 24, 2022
kainino0x
kainino0x approved these changes Aug 25, 2022
View reviewed changes
@litherum @kainino0x
Update spec/index.bs
64bc287 
Co-authored-by: Kai Ninomiya <kainino1@gmail.com>
@litherum litherum enabled auto-merge (squash) August 25, 2022 02:52
@litherum litherum merged commit db0151e into gpuweb:main Aug 25, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 25, 2022

Previews, as seen when this build job started (64bc287):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

Kangz
Kangz reviewed Aug 26, 2022
View reviewed changes
juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 29, 2022
@juj
IDL: Add GPUSupportedLimits.maxBufferSize. gpuweb/gpuweb#3366
c4e9def
@kainino0x kainino0x added needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet tacit resolution queue Editors have agreed and intend to land if no feedback is given and removed tacit resolution queue Editors have agreed and intend to land if no feedback is given labels Aug 30, 2022
@litherum litherum deleted the maxBufferSize branch September 7, 2022 07:34
@kdashg
Copy link
Contributor

kdashg commented Sep 9, 2022

GPU Web 2022-09-07/08 APAC-timed
  • KN: what would it take to raise this limit? Is it worth it?
  • KN: think the only device with this limit is iPhone 6 or that hardware generation.
  • MM: it's AMD GPUs (in Metal) that impose this limit; there's more than one of them. iPhone 6 has a higher limit than that.
  • KN: numbers from Stack Overflow reported 256 MB on iPhone 6 for this limit.
  • KG: would expect this to be in the Metal feature set tables.
  • MM: if you run a process and the process asks for the limit and you go to bed, wake up in the morning, run the app again - the limit can be different.
  • KG: so there is logic associated with this.
  • KG: with that in mind - the 256 MB was what we expected we could always support.
  • MM: yes.
  • KG: possibly good candidate - have a lower bar for this - when we want to have things not stuck at the minimum/maximum values. How much we want things to float around.
  • KN: can you clarify?
  • KG: don't remember our consensus - was a strong push for limits to be equal to the required value (min/max). This one, since we want it to be bigger and sometimes dynamic - could be valuable to say, it'll be at least 256, and may be bigger.
  • KN: without asking?
  • KG: what's actionable? Maybe - proposal to raise this limit, can negotiate there.
  • KN: 2 ways we can raise it. 1) drop device support. 2) say it's OK some devices will have OOMs between 256 MB and whatever we raise it to. (2) not a great option.
  • MM: third option - back a WebGPU buffer by two platform buffers.
  • KG / KN: sure.
  • KG: would be useful to have proposal to make this bigger. 256 MB sounded like a starting point. Want to solidify things more now. Well positioned if we wanted to make this change. Make it bigger either dynamically or statically.
  • KN: q for right now - could we drop a couple of devices that are very old? Do we even run on devices that have this limit?
  • MM: I don't think we should drop them - don't remember the model names, but they're big fancy Macs that people spend thousands of dollars on. Should make WebGPU run on them.
  • KG: useful info.
  • KN: yes, useful, can leave it as it is.

@kdashg kdashg removed the tacit resolution queue Editors have agreed and intend to land if no feedback is given label Sep 14, 2022
@lokokung
Copy link
Contributor

lokokung commented Dec 1, 2022

Removing 'needs-cts-issue' label.

Added draft issues:

  • val: buffer,create:limit (Needs actual validation tests and limit for the new value)
  • op: buffers,map_oom (Needs re-review because of the new limit)

Updated draft issue val: capability_checks,limits to encompass this issue in the CTS. The former also notes that the limit needs to be added to kLimits.

@lokokung lokokung removed the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Dec 1, 2022
@lokokung lokokung mentioned this pull request Jan 24, 2023
Closed
@greggman greggman mentioned this pull request Jan 25, 2023
Closed
@kainino0x kainino0x mentioned this pull request Mar 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kangz Kangz Kangz left review comments

@kainino0x kainino0x kainino0x approved these changes

@toji toji Awaiting requested review from toji

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Limit for the maximum buffer size

5 participants

@litherum @kdashg @lokokung @Kangz @kainino0x