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

Specify default offset and size for getMappedRange #908

Merged
merged 3 commits into from
Jul 22, 2020

Conversation

kunalmohan
Copy link
Contributor

@kunalmohan kunalmohan commented Jul 7, 2020

Defaults are specified w.r.t buffer mapping_range.
While setting the default size, offset is checked with only lower bound inclusion to prevent zero-sized array buffers to be returned.


Preview | Diff

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@kunalmohan
Copy link
Contributor Author

kunalmohan commented Jul 7, 2020

mapAsync offset should be a multiple of 4 whereas getMappedRange offset should be a multiple of 8. IIUC, the user will never be able to access the first 4 bytes of the mapping_range in case mapAsync offset is multiple of 4 but not 8. (maybe I'm wrong here)
Also if we keep the mapAsync offset to be multiple of 4, we should change the getMappedRange rangeOffset to-

If offset is unspecified, let rangeOffset be smallest multiple of 8 greater than or equal to this.[[mapping_range]][0].

for validation rules to apply.

I have added an issue for the same at the moment.

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@kainino0x
Copy link
Contributor

For the meeting, here's a concrete version of the proposal that I made above:

Two overloads:

ArrayBuffer getMappedRange();
ArrayBuffer getMappedRange(GPUSize64 offset, GPUSize64 size);

Eliminates ambiguities about what size=0 means, etc. and eliminates an (honestly useless) overload of this function.

@kainino0x
Copy link
Contributor

kainino0x commented Jul 20, 2020

Resolution: getMappedRange(optional GPUSize64 offset = 0, optional GPUSize64 size), with:

  • offset is relative to whole buffer
  • offset defaults to 0
  • size defaults to buffer.size - offset

@kunalmohan
Copy link
Contributor Author

So in case offset isn't specified and this.[[mapping_range]][0] > 0, we throw Operation Error?

@kainino0x
Copy link
Contributor

Correct!

@kainino0x
Copy link
Contributor

Small unresolved thing (didn't realize before): we should figure out whether it's

  • optional GPUSize64 size, undefined means buffer.size, 0 means 0?
  • optional GPUSize64 size = 0, 0 means buffer.size - offset

We can talk about this at the editors meeting in an hour.

@kainino0x
Copy link
Contributor

  • optional GPUSize64 size, undefined means buffer.size, 0 means 0?

This one, to keep the signalling out of band.

@kainino0x
Copy link
Contributor

@kunalmohan Do you want to take care of this revision work, or would you like someone else to take care of it? Editors would be happy to take it.

@kunalmohan
Copy link
Contributor Author

I would like to take a stab at it :)

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@kainino0x
Copy link
Contributor

You can view my nits commit without whitespace changes: 369b83a?diff=split&w=1

@kunalmohan
Copy link
Contributor Author

kunalmohan commented Jul 21, 2020

This means that if size is specified, but offset is >= this.[[size]], then we use this.[[size]].

Not sure what your concern here was. I would interpret such a case to be using |rangeSize| = |size| (rather than this.[[size]]) which then throws validation Error because |offset| + |rangeSize| > this.[[mapping_range]][1]. But it certainly sounds more clear with your changes :)

Also, not completely sure but we might consider changing the language in mapAsync() as well if this is the case. (when the size is 0 there)

@kainino0x
Copy link
Contributor

This means that if size is specified, but offset is >= this.[[size]], then we use this.[[size]].

Not sure what your concern here was.

As written:

If |size| is unspecified and |offset| is less than this.{{[[size]]}},
let |rangeSize| be this.{{[[size]]}} - |offset|. Otherwise, let |rangeSize| be |size|.

it would mean:

int rangeSize;
if (size == undefined && offset < this.size) {
  rangeSize = this.size - offset;
} else {
  rangeSize = size; // <- size could be undefined here
}

but the "offset > this.size" case has to be an error.

@kainino0x
Copy link
Contributor

You're right about mapAsync needing a fix too.

@kunalmohan
Copy link
Contributor Author

This means that if size is specified, but offset is >= this.[[size]], then we use this.[[size]].

Not sure what your concern here was.

As written:

If |size| is unspecified and |offset| is less than this.{{[[size]]}},
let |rangeSize| be this.{{[[size]]}} - |offset|. Otherwise, let |rangeSize| be |size|.

it would mean:

int rangeSize;
if (size == undefined && offset < this.size) {
  rangeSize = this.size - offset;
} else {
  rangeSize = size; // <- size could be undefined here
}

but the "offset > this.size" case has to be an error.

Ohh..I see. Thank you for clarifynig!

@kainino0x
Copy link
Contributor

I would interpret such a case to be using |rangeSize| = |size| (rather than this.[[size]]) which then throws validation Error because |offset| + |rangeSize| > this.[[mapping_range]][1].

Oops, I misread this - I think you're right the original case I pointed out would have been caught correctly. But the opposite issue (size undefined and offset ≥ this.size) was still there.

Copy link
Contributor

@kvark kvark left a comment

Choose a reason for hiding this comment

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

One last thing from me

spec/index.bs Show resolved Hide resolved
@kainino0x kainino0x merged commit b31071c into gpuweb:main Jul 22, 2020
@kunalmohan kunalmohan deleted the get-mapped-range branch July 22, 2020 18:22
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

5 participants