Inconsistency between MLGraphBuilder and MLBuffer construction

[reillyeon]

An MLGraphBuilder and an MLBuffer are both associated with an MLContext however MLGraphBuilder is created with a normal constructor while MLBuffer is created by the createBuffer() method on MLContext.

We should consider removing createBuffer() and defining a normal constructor for MLBuffer with the same semantics as MLGraphBuilder (or add a createBuilder() method, but I'd prefer the former option).

[bbernhar]

I think #529 is related. createBuffer() is off the MLContext because it associated the construction operation on the context (or device) timeline. MLGraphBuilder is timeline-agnostic, it's construction does NOT rely or depend on context/device operations. WebNN's construction behavior should be described in terms of timelines because a normal constructor does not define any order of operations or what MLContext state MLGraphBuilder has access to (eg. cannot modify MLBuffer).

[RafaelCintron]

Factory methods on "context" objects is the existing, predominant pattern in 2D canvas, WebGL and WebGPU. Buffers, textures, bind groups, bind group layouts, shader modules, gradients, image data, and many more are all factory methods on the created context.

At the same time, the official W3C design principles document advises that objects should have constructors when possible unless the object is a base class of other objects or requires special privileges, neither of which really applies in our case.

For synchronous buffer creation, the same object is created whether the web developer types myBuffer = new MLBuffer(context, params) vs. myBuffer = context.createBuffer(params). Both approaches require a context to be created first and for the buffer to be associated with the context.

For objects which are synchronously created, using a constructor seems preferable as it's a guaranteed way to get a valid object. If we later decide to add async buffer creation, we can add a promise returning createBuffer API on the context and not obsess about which factory method to add the word "sync" vs "async" to.

[zolkis]

Just for the record, a long time ago, in a similar discussion, I got the following advice from @domenic (well documented in the design principles spec). It's in line with @RafaelCintron's comment above. Some of these could be applicable here, whether we move relevant stuff towards constructors, or towards factories representations.

• [ The root object should not be constructible. It represents the UA's magic ability to discover things, from what I understand, similar to how the Navigator object represents the UA's magic ability to do a bunch of stuff. A namespace might be a good replacement here, if it truly has no state. ]
• Avoid constructor overloads. A true constructor should be something that directly copies the given essential data into internal fields. If there is a way to infer the essential data from some other data, then that should be a factory.
• The idea of using a builder pattern (first create an X, then call another function on it to turn into an X-with-UA-magic) is rather unidiomatic in JavaScript. The better way to represent something without UA magic is just a dictionary. Having the same class represent two very distinct things is not great. So, although I am normally in favor of constructors, from what I can gather here, it seems like they are not being used to actually construct the object with all of its relevant data; they are being used as part of a builder pattern of some kind.

So if MLBuffer is constructible in one phase and usable after that without further specialization, using a constructor is fine and recommended.

But before deciding that, as @bbernhar pointed out, we should clarify timelines (#529), and capture their relationship vs. object creation. If creating MLBuffer will ever involve any background operations or UA-specific initializations, IMHO it should be created by a factory method.

[bbernhar]

If creating MLBuffer will ever involve any background operations or UA-specific initializations, IMHO it should be created by a factory method.

+1. While createBuffer() appears to be a synchronous JS API operation, its construction could be a background (or async) operation. This is the case in our current MLBuffer implementation. Even if MLBuffer construction was sync, buffer data initialization could still require a "clear" command off the device, which means it must respect the order of operations on the same context. It's not safe to assume MLBuffer() is just creating a valid object, it can be more complex.

[RafaelCintron]

From the web developer's perspective, when they create an MLBuffer via the constructor, they get back an object they can use in subsequent API calls right away. Whether "actual" construction happens on a background thread or in a different process is an implementation detail of the browser.

However, as I said previously, there's prior art in other web APIs to have factory methods for context-y objects.

[bbernhar]

To address this issue, I propose we have createBuffer() return a promise.

It would:

1. Make MLBuffer construction consistent.
2. Offer a better OOM debugging experience.

Any objections? 👍== OK with change.

@reillyeon @a-sully @RafaelCintron @zolkis @huningxin

[a-sully]

As I mentioned on this comment thread I'm not opposed to making createBuffer() async, but it seems like this is being pursued mainly on the assumption that createBuffer() is the only method which may fail in a recoverable way - i.e. that by making createBuffer() async we can avoid the need for async error reporting (à la #477), since all async errors would be unrecoverable. I'm skeptical that's true...

...this isn't necessarily a blocking concern, though. And in the meantime (while we don't yet have #477) it is nice to know whether createBuffer() succeeded. So here's my tentative +1 👍

[bbernhar]

@a-sully

I'm skeptical that's true...

It's worth pointing out that since MLBuffer(usage) assumes the WebNN runtime/driver will manage memory (on the web developer's behalf), we should also expect WebNN implementations to start caching and/or pre-allocating heaps for MLBuffer(s) across the context. This means OOM will be recoverable by purging unused memory. If the web developer is 5MB from the OS commit limit, OOM is unlikely recoverable but if some buffer, GBs in size, causes OOM, that's fully recoverable. This is what DML was designed for (note: bolded).

Tasks that are left to your discretion include: transcribing the model; simplifying and optimizing your layers; loading weights; resource allocation, binding, memory management (just as with Direct3D 12); and execution of the graph.