gemini-code-assist[bot] commented on code in PR #19673:
URL: https://github.com/apache/tvm/pull/19673#discussion_r3354189269
##########
web/src/opfs_store.ts:
##########
@@ -48,61 +56,87 @@ interface OPFSStoreMetadata {
contentType?: string;
}
+interface OPFSStoredEntry {
+ payloadHandle: OPFSFileHandle;
+ metadata: OPFSStoreMetadata;
+}
+
+interface OPFSSyncAccessHandle {
+ getSize(): number;
+ read(buffer: BufferSource, options?: { at?: number }): number;
+ write(buffer: BufferSource, options?: { at?: number }): number;
+ truncate(size: number): void;
+ flush(): void;
+ close(): void;
+}
+
+type OPFSGlobalScope = typeof globalThis & {
+ DedicatedWorkerGlobalScope?: new () => object;
+ FileSystemFileHandle?: {
+ prototype?: {
+ createSyncAccessHandle?: unknown;
+ };
+ };
+};
+
const HASH_ALGORITHM = "SHA-256";
const OPFS_STORE_ROOT_DIRECTORY = "tvmjs-opfs-store";
export class OPFSStore {
private readonly scope: string;
+ private accessMode: OPFSEffectiveAccessMode;
private directoryPromise?: Promise<OPFSDirectoryHandle>;
- constructor(scope: string) {
+ constructor(scope: string, accessMode: OPFSAccessMode = "async") {
this.scope = scope;
+ this.accessMode = OPFSStore.resolveAccessMode(accessMode);
}
static isAvailable(): boolean {
const storage = OPFSStore.getStorageManager();
return storage !== undefined && typeof storage.getDirectory === "function";
}
+ private static resolveAccessMode(
+ accessMode: OPFSAccessMode,
+ ): OPFSEffectiveAccessMode {
+ if (accessMode !== "auto") {
+ return accessMode;
+ }
+ return OPFSStore.isDedicatedWorkerWithSyncAccessHandle()
+ ? "sync"
+ : "async";
+ }
+
async has(url: string): Promise<boolean> {
- return (await this.read(url)) !== undefined;
+ return (await this.getExistingEntry(url)) !== undefined;
}
async read(url: string): Promise<Response | undefined> {
- const directory = await this.getScopedDirectory();
- const baseName = await this.hashUrl(url);
- const dataHandle = await this.getFileHandleIfExists(
- directory,
- `${baseName}.bin`,
- false,
- );
- if (dataHandle === undefined) {
+ const entry = await this.getExistingEntry(url);
+ if (entry === undefined) {
return undefined;
}
- const dataBlob = await dataHandle.getFile();
- const metadataHandle = await this.getFileHandleIfExists(
- directory,
- `${baseName}.meta.json`,
- false,
- );
- let metadata: OPFSStoreMetadata | undefined = undefined;
- if (metadataHandle !== undefined) {
- metadata = await this.readMetadata(metadataHandle);
- if (metadata?.url !== undefined && metadata.url !== url) {
- throw new Error("OPFSStore: metadata URL does not match key URL.");
- }
- }
+ const payload = await this.readPayload(entry.payloadHandle);
const headers =
- metadata?.contentType !== undefined
- ? { "content-type": metadata.contentType }
+ entry.metadata.contentType !== undefined
+ ? { "content-type": entry.metadata.contentType }
: undefined;
- return new Response(dataBlob, headers ? { headers } : undefined);
+ return new Response(payload, headers ? { headers } : undefined);
+ }
Review Comment:
### Memory and Performance Regression in Async Mode
In the new implementation, `read(url)` always reads the entire payload into
memory as an `ArrayBuffer` via `readPayload(entry.payloadHandle)`, even when
`accessMode` is set to `"async"`.
In contrast, the original implementation returned a `Response` wrapping the
lazy `Blob` directly (`await dataHandle.getFile()`). Wrapping a `Blob` in a
`Response` is highly memory-efficient because the browser does not load the
file content into memory until it is actually read (e.g., via `.arrayBuffer()`
or `.json()`). For large model files (which can be hundreds of megabytes or
gigabytes), loading them entirely into memory as `ArrayBuffer`s when they could
be handled as lazy `Blob`s is a major memory bottleneck and can easily cause
Out-Of-Memory (OOM) crashes on mobile devices or memory-constrained
environments.
To preserve the memory efficiency of the `"async"` mode, we should return
the lazy `Blob` directly when `accessMode === "async"`.
```typescript
async read(url: string): Promise<Response | undefined> {
const entry = await this.getExistingEntry(url);
if (entry === undefined) {
return undefined;
}
const headers =
entry.metadata.contentType !== undefined
? { "content-type": entry.metadata.contentType }
: undefined;
if (this.accessMode === "async") {
const blob = await entry.payloadHandle.getFile();
return new Response(blob, headers ? { headers } : undefined);
}
const payload = await this.readPayload(entry.payloadHandle);
return new Response(payload, headers ? { headers } : undefined);
}
```
##########
web/src/opfs_store.ts:
##########
@@ -196,15 +262,104 @@ export class OPFSStore {
}
}
- private async writeFile(
+ private async writeMetadata(
handle: OPFSFileHandle,
- data: Blob | BufferSource | string,
+ metadata: OPFSStoreMetadata,
): Promise<void> {
const writable = await handle.createWritable();
- await writable.write(data);
+ await writable.write(new TextEncoder().encode(JSON.stringify(metadata)));
await writable.close();
}
+ private async readPayload(handle: OPFSFileHandle): Promise<ArrayBuffer> {
+ const syncHandle = await this.openSyncAccessHandle(handle, "read-only");
+ return syncHandle !== undefined
+ ? this.readPayloadWithSyncHandle(syncHandle)
+ : (await handle.getFile()).arrayBuffer();
+ }
+
+ private async writePayload(
+ handle: OPFSFileHandle,
+ response: Response,
+ ): Promise<void> {
+ const syncHandle = await this.openSyncAccessHandle(handle, "readwrite");
+ if (syncHandle !== undefined) {
+ await this.writePayloadWithSyncHandle(syncHandle, response);
+ return;
+ }
+ await this.writePayloadWithWritable(handle, response);
+ }
+
+ private async writePayloadWithWritable(
+ handle: OPFSFileHandle,
+ response: Response,
+ ): Promise<void> {
+ const writable = await handle.createWritable();
+ if (response.body !== null) {
+ await response.body.pipeTo(writable);
+ } else {
+ await writable.write(await response.arrayBuffer());
+ await writable.close();
+ }
+ }
+
+ private readPayloadWithSyncHandle(
+ syncHandle: OPFSSyncAccessHandle,
+ ): ArrayBuffer {
+ try {
+ const size = syncHandle.getSize();
+ const payload = new ArrayBuffer(size);
+ syncHandle.read(payload, { at: 0 });
+ return payload;
+ } finally {
+ syncHandle.close();
+ }
+ }
+
+ private async writePayloadWithSyncHandle(
+ syncHandle: OPFSSyncAccessHandle,
+ response: Response,
+ ): Promise<void> {
+ try {
+ syncHandle.truncate(0);
+ let offset = 0;
+ if (response.body !== null) {
+ const reader = response.body.getReader();
+ try {
+ while (true) {
+ const { done, value } = await reader.read();
+ if (done) {
+ break;
+ }
+ syncHandle.write(value, { at: offset });
+ offset += value.byteLength;
+ }
+ } finally {
+ reader.releaseLock();
+ }
+ } else {
+ const payload = await response.arrayBuffer();
+ syncHandle.write(payload, { at: 0 });
+ }
Review Comment:
### Compatibility with Older Browser Implementations of OPFS
For the same reason as in `readPayloadWithSyncHandle`, wrapping the
`ArrayBuffer` in a `Uint8Array` before calling `write` ensures full backward
compatibility with older browser implementations of the OPFS API.
```suggestion
} else {
const payload = await response.arrayBuffer();
syncHandle.write(new Uint8Array(payload), { at: 0 });
}
```
##########
web/src/opfs_store.ts:
##########
@@ -196,15 +262,104 @@ export class OPFSStore {
}
}
- private async writeFile(
+ private async writeMetadata(
handle: OPFSFileHandle,
- data: Blob | BufferSource | string,
+ metadata: OPFSStoreMetadata,
): Promise<void> {
const writable = await handle.createWritable();
- await writable.write(data);
+ await writable.write(new TextEncoder().encode(JSON.stringify(metadata)));
await writable.close();
}
+ private async readPayload(handle: OPFSFileHandle): Promise<ArrayBuffer> {
+ const syncHandle = await this.openSyncAccessHandle(handle, "read-only");
+ return syncHandle !== undefined
+ ? this.readPayloadWithSyncHandle(syncHandle)
+ : (await handle.getFile()).arrayBuffer();
+ }
+
+ private async writePayload(
+ handle: OPFSFileHandle,
+ response: Response,
+ ): Promise<void> {
+ const syncHandle = await this.openSyncAccessHandle(handle, "readwrite");
+ if (syncHandle !== undefined) {
+ await this.writePayloadWithSyncHandle(syncHandle, response);
+ return;
+ }
+ await this.writePayloadWithWritable(handle, response);
+ }
+
+ private async writePayloadWithWritable(
+ handle: OPFSFileHandle,
+ response: Response,
+ ): Promise<void> {
+ const writable = await handle.createWritable();
+ if (response.body !== null) {
+ await response.body.pipeTo(writable);
+ } else {
+ await writable.write(await response.arrayBuffer());
+ await writable.close();
+ }
+ }
+
+ private readPayloadWithSyncHandle(
+ syncHandle: OPFSSyncAccessHandle,
+ ): ArrayBuffer {
+ try {
+ const size = syncHandle.getSize();
+ const payload = new ArrayBuffer(size);
+ syncHandle.read(payload, { at: 0 });
+ return payload;
+ } finally {
+ syncHandle.close();
+ }
+ }
Review Comment:
### Compatibility with Older Browser Implementations of OPFS
In some older browser implementations of the OPFS API (e.g., Chrome
102-107), `FileSystemSyncAccessHandle.read` and `write` only accepted
`ArrayBufferView` (such as `Uint8Array`), and passing a raw `ArrayBuffer`
directly would throw a `TypeError`.
Wrapping the `ArrayBuffer` in a `Uint8Array` is fully backward-compatible,
safe, and has zero overhead.
```suggestion
private readPayloadWithSyncHandle(
syncHandle: OPFSSyncAccessHandle,
): ArrayBuffer {
try {
const size = syncHandle.getSize();
const payload = new ArrayBuffer(size);
syncHandle.read(new Uint8Array(payload), { at: 0 });
return payload;
} finally {
syncHandle.close();
}
}
```
##########
web/src/opfs_store.ts:
##########
@@ -196,15 +262,104 @@ export class OPFSStore {
}
}
- private async writeFile(
+ private async writeMetadata(
handle: OPFSFileHandle,
- data: Blob | BufferSource | string,
+ metadata: OPFSStoreMetadata,
): Promise<void> {
const writable = await handle.createWritable();
- await writable.write(data);
+ await writable.write(new TextEncoder().encode(JSON.stringify(metadata)));
await writable.close();
}
+ private async readPayload(handle: OPFSFileHandle): Promise<ArrayBuffer> {
+ const syncHandle = await this.openSyncAccessHandle(handle, "read-only");
+ return syncHandle !== undefined
+ ? this.readPayloadWithSyncHandle(syncHandle)
+ : (await handle.getFile()).arrayBuffer();
+ }
+
+ private async writePayload(
+ handle: OPFSFileHandle,
+ response: Response,
+ ): Promise<void> {
+ const syncHandle = await this.openSyncAccessHandle(handle, "readwrite");
+ if (syncHandle !== undefined) {
+ await this.writePayloadWithSyncHandle(syncHandle, response);
+ return;
+ }
+ await this.writePayloadWithWritable(handle, response);
+ }
+
+ private async writePayloadWithWritable(
+ handle: OPFSFileHandle,
+ response: Response,
+ ): Promise<void> {
+ const writable = await handle.createWritable();
+ if (response.body !== null) {
+ await response.body.pipeTo(writable);
+ } else {
+ await writable.write(await response.arrayBuffer());
+ await writable.close();
+ }
+ }
+
+ private readPayloadWithSyncHandle(
+ syncHandle: OPFSSyncAccessHandle,
+ ): ArrayBuffer {
+ try {
+ const size = syncHandle.getSize();
+ const payload = new ArrayBuffer(size);
+ syncHandle.read(payload, { at: 0 });
+ return payload;
+ } finally {
+ syncHandle.close();
+ }
+ }
+
+ private async writePayloadWithSyncHandle(
+ syncHandle: OPFSSyncAccessHandle,
+ response: Response,
+ ): Promise<void> {
+ try {
+ syncHandle.truncate(0);
+ let offset = 0;
+ if (response.body !== null) {
+ const reader = response.body.getReader();
+ try {
+ while (true) {
+ const { done, value } = await reader.read();
+ if (done) {
+ break;
+ }
+ syncHandle.write(value, { at: offset });
+ offset += value.byteLength;
+ }
+ } finally {
+ reader.releaseLock();
+ }
+ } else {
+ const payload = await response.arrayBuffer();
+ syncHandle.write(payload, { at: 0 });
+ }
+ syncHandle.flush();
+ } finally {
+ syncHandle.close();
+ }
+ }
+
+ private async openSyncAccessHandle(
+ handle: OPFSFileHandle,
+ mode: OPFSSyncAccessHandleMode,
+ ): Promise<OPFSSyncAccessHandle | undefined> {
+ if (this.accessMode === "async") {
+ return undefined;
+ }
+ if (typeof handle.createSyncAccessHandle !== "function") {
+ throw this.createSyncUnavailableError();
+ }
+ return await handle.createSyncAccessHandle({ mode });
+ }
Review Comment:
### Concurrency Lock Failures in OPFS Sync Access Handles
Calling `createSyncAccessHandle` on a file handle that already has an active
sync access handle open (or is undergoing other file system operations) will
throw a `NoModificationAllowedError` `DOMException`.
Because `ArtifactCacheTemplate` operations can be called concurrently (e.g.,
multiple concurrent reads or writes to the same shard), this error will
propagate and crash the read/write operation.
To make the OPFS store extremely resilient, we should wrap
`createSyncAccessHandle` in a `try-catch` block and return `undefined` on
error. This allows the caller (`readPayload` or `writePayload`) to
automatically and gracefully fall back to the highly-compatible and
concurrency-safe async methods (`getFile()` or `createWritable()`).
```typescript
private async openSyncAccessHandle(
handle: OPFSFileHandle,
mode: OPFSSyncAccessHandleMode,
): Promise<OPFSSyncAccessHandle | undefined> {
if (this.accessMode === "async") {
return undefined;
}
if (typeof handle.createSyncAccessHandle !== "function") {
throw this.createSyncUnavailableError();
}
try {
return await handle.createSyncAccessHandle({ mode });
} catch (err) {
return undefined;
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
