Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

Having to store maxByteLength is not very useful #18

Closed
marjakh opened this issue Jan 20, 2021 · 9 comments · Fixed by #25
Closed

Having to store maxByteLength is not very useful #18

marjakh opened this issue Jan 20, 2021 · 9 comments · Fixed by #25

Comments

@marjakh
Copy link

marjakh commented Jan 20, 2021

Afaics the maxByteLength passed as a parameter to the ctor is only needed for implementing the following behavior:

(Resizable|GrowableShared)ArrayBuffer.prototype.resize:

If newByteLength > O.[[ArrayBufferMaxByteLength]], throw a RangeError exception.

It's not needed for anything else. Typically an implementation would allocate a memory area of some length (maybe maxByteLength rounded up to the page length or maybe something else - it's allowed to ignore maxByteLength altogether per spec!) and then store the amount of memory allocated. In resize, we could check against what we have allocated, not against maxByteLength. (And for completeness, noting that resize is allowed to throw even if the newByteLength <= maxByteLength.)

The current spec leads to resize throwing even in cases where we could resize in place (because we originally allocated more than maxByteLength). That feels unnecessary too.

I think the right way to "fix" this (if deemed undesirable) would be simply to remove the above mentioned "if" and not storing maxByteLength. That would internalize all throwing behavior and heuristics for which size to actually allocate to CreateByteDataBlock.

@syg
Copy link
Collaborator

syg commented Jan 22, 2021

It's not needed for anything else.

[[ArrayBufferMaxByteLength]] is also used as a brand check for RAB and GSABs, in addition to storing the max byte length.

The current spec leads to resize throwing even in cases where we could resize in place (because we originally allocated more than maxByteLength). That feels unnecessary too.

I'm not sure I understand this. Are you saying there are cases where you want a resize(N) to succeed if N > maxByteLength? Originally allocating more than maxByteLength is fine, but it's still desirable to make the resize behavior predictable by making maxByteLength the observable max, even if the actually allocated data is larger.

I guess you're asking that maxByteLength becomes a hint where an implementation is allowed to round it up (but not down) in an implementation-defined way?

@marjakh
Copy link
Author

marjakh commented Jan 22, 2021

The implementation is also allowed to round down (indirectly), since resize() & grow() are always allowed to throw.

Implementations would likely do this:
ctor(byteLength, maxByteLength) {
this.reserved_size = ImplementationDefinedHeuristic(maxByteLength); // might return something bigger or smaller than maxByteLength
allocate memory of this.reserved_size
}

resize(newByteLength) {
if (newByteLength > this.reserved_size) throw(...);
...
}

So, the implementations are already allowed to ignore maxByteLength (or feed it into some heuristic) when deciding which size to allocate. The only thing which forces them to store it is the requirement that resize() must throw if newByteLength > maxByteLength.

Fulfilling that requirement forces the implementation to store an additional data member. Without that requirement, the implementation can just store the return value of ImplementationDefinedHeuristic(maxByteLength).

To be clear, I'm suggesting getting rid of the feature that resize & grow must throw if newByteLength > maxByteLength (where maxByteLength is what was passed to the ctor). The rationale is that having that feature requires the implementation to store one additional data member -> IMO not worth it.

Spec-wise, this could be achieved e.g., by saying

Let O.[[ArrayBufferMaxByteLength]] be ImplementationDefinedHeuristic(maxByteLength).

and everything else would stay the same.

@syg
Copy link
Collaborator

syg commented Jan 22, 2021

Thanks for the clarification.

Spec-wise, this could be achieved e.g., by saying

Let O.[[ArrayBufferMaxByteLength]] be ImplementationDefinedHeuristic(maxByteLength).

This is better than what I initially understood your suggestion to be. It's saying that during construction time, the implementation is allowed to do some rounding, but importantly, the rounded number becomes observable via .prototype.byteLength, and then resize() throws if it's bigger than the rounded number. As long as the rounding (i.e. to a page size multiple) becomes immediately visible, this seems more reasonable.

On the flip side, this would expose more engine internals, which wouldn't be desirable.

For example, suppose an implementation decides to round up to page size multiples for large buffers, and don't do that for small buffers. This allocation strategy becomes immediately observable and might make it harder to change allocation strategies down the road, if there's now code out there that depends on no rounding happening for small buffers. (Maybe also implications for fingerprinting and security, but I don't know much about that space.)

@marjakh
Copy link
Author

marjakh commented Jan 25, 2021

Hmm, why would the byteLength change? That was not part of my suggestion originally. Did you mean maxByteLength there?

There are several options what we could do:

  1. Let O.[[ArrayBufferMaxByteLength]] be ImplementationDefinedHeuristic(maxByteLength). + keep the maxByteLength getter. (My orig suggestion.) This way the user can at least use the getter to get useful information. The current return value is not useful, as it's only about remembering the param passed to the ctor. It can't be used about reasoning about whether resize() throws or not (except one way: if bigger, resize will always throw). In particular, it doesn't give the opposite guarantee: if I pass something smaller than that, resize won't throw.

  2. A version which tries to be unobservable: Let O.[[ArrayBufferMaxByteLength]] be ImplementationDefinedHeuristic(maxByteLength). + remove the maxByteLength getter. This way the user can't observe the heuristic other than trying to resize() and seeing when it throws or not (which is also affected by other things like whether we're out of memory).

  3. A version which tries to make the maxByteLength getter even more useful: Let O.[[ArrayBufferMaxByteLength]] be ImplementationDefinedHeuristic(maxByteLength) + keep the maxByteLength getter + make resize() give the guarantee that if new_length <= maxByteLength, it won't throw, and if new_length > maxByteLength, it will throw.

@syg
Copy link
Collaborator

syg commented Jan 25, 2021

Hmm, why would the byteLength change? That was not part of my suggestion originally. Did you mean maxByteLength there?

Err, sorry, yes I meant maxByteLength.

@syg
Copy link
Collaborator

syg commented Jan 25, 2021

I favor option 1. Option 3 is harder because it precludes certain legitimate implementation strategies, like moving the backing buffer on every resize, even shrinks. Embedded devices with very limited memory, for example, may opt for that strategy so that every resize results in a compaction.

@ljharb
Copy link
Member

ljharb commented Jan 25, 2021

What's wrong with option 2? Making it unobservable seems like it preserves the most freedom while minimizing the chance users will intuit the wrong thing.

@syg
Copy link
Collaborator

syg commented Jan 25, 2021

From my perspective the main undesirability of option 2 is that maxByteLength still seems useful. For applications dealing with multiple buffers, the ability to see the total reserved virtual memory without physically trying to commit the pages and catching thrown errors seems useful.

In particular @ljharb, this reasoning seems similar to me as why we want #foo in obj despite being able to do try { obj.#foo; } catch (e) { ... }.

@ljharb
Copy link
Member

ljharb commented Jan 26, 2021

Makes sense, if it's useful then option 2 wouldn't be a good pick.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants