gemini-code-assist[bot] commented on code in PR #19673:
URL: https://github.com/apache/tvm/pull/19673#discussion_r3370384215
##########
web/src/opfs_store.ts:
##########
@@ -166,43 +283,178 @@ export class OPFSStore {
return this.directoryPromise;
}
- private async readMetadata(
+ private async readRecord(
fileHandle: OPFSFileHandle,
- ): Promise<OPFSStoreMetadata | undefined> {
+ ): Promise<OPFSStoreRecord | undefined> {
try {
const text = await (await fileHandle.getFile()).text();
const parsed = JSON.parse(text);
if (
parsed === undefined ||
parsed === null ||
typeof parsed !== "object" ||
- typeof parsed.url !== "string"
+ typeof parsed.url !== "string" ||
+ !Number.isSafeInteger(parsed.nbytes) ||
+ parsed.nbytes < 0
) {
- throw new Error("OPFSStore: invalid metadata format.");
+ return undefined;
}
- const metadata: OPFSStoreMetadata = {
+ const record: OPFSStoreRecord = {
url: parsed.url,
+ nbytes: parsed.nbytes,
};
if (typeof parsed.contentType === "string") {
- metadata.contentType = parsed.contentType;
+ record.contentType = parsed.contentType;
}
- return metadata;
+ return record;
} catch (err) {
- if (this.isNotFoundError(err)) {
- // Treat metadata disappearance between lookup and read as a cache miss
+ if (
+ OPFSStore.getErrorName(err) === "SyntaxError" ||
+ this.handleCacheMissStateError(err)
+ ) {
return undefined;
}
throw err;
}
}
- private async writeFile(
+ private getResponseInit(
+ record: OPFSStoreRecord,
+ ): ResponseInit | undefined {
+ return record.contentType !== undefined
+ ? { headers: { "content-type": record.contentType } }
+ : undefined;
+ }
+
+ private async writeRecord(
handle: OPFSFileHandle,
- data: Blob | BufferSource | string,
+ record: OPFSStoreRecord,
): Promise<void> {
const writable = await handle.createWritable();
- await writable.write(data);
+ await writable.write(new TextEncoder().encode(JSON.stringify(record)));
+ 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 hasExpectedPayloadSize(
+ entry: OPFSStoredEntry,
+ ): Promise<boolean> {
+ const blob = await entry.payloadHandle.getFile();
+ return blob.size === entry.record.nbytes;
+ }
+
+ private async writePayload(
+ handle: OPFSFileHandle,
+ response: Response,
+ ): Promise<number> {
+ const syncHandle = await this.openSyncAccessHandle(handle, "readwrite");
+ if (syncHandle !== undefined) {
+ return this.writePayloadWithSyncHandle(syncHandle, response);
+ }
+ return this.writePayloadWithWritable(handle, response);
+ }
+
+ private async writePayloadWithWritable(
+ handle: OPFSFileHandle,
+ response: Response,
+ ): Promise<number> {
+ const writable = await handle.createWritable();
+ if (response.body !== null) {
+ let nbytes = 0;
+ const reader = response.body.getReader();
+ try {
+ while (true) {
+ const { done, value } = await reader.read();
+ if (done) {
+ break;
+ }
+ await writable.write(value);
+ nbytes += value.byteLength;
+ }
+ } finally {
+ reader.releaseLock();
+ }
+ await writable.close();
+ return nbytes;
+ }
+ const payload = await response.arrayBuffer();
+ await writable.write(payload);
await writable.close();
+ return payload.byteLength;
+ }
Review Comment:
If an error occurs during streaming or writing (for example, if the network
connection is interrupted or the reader throws an error), the `writable` stream
is never closed or aborted. In the Origin Private File System (OPFS), an
unclosed `FileSystemWritableFileStream` retains an exclusive lock on the file.
This can cause subsequent read or write operations on the same file to fail
with a `NoModificationAllowedError` until the page is reloaded or garbage
collection runs.
We should wrap the write operations in a `try...catch` block and ensure that
`writable.abort()` is called if an error occurs, which safely releases the lock
and discards any partial temporary file.
```typescript
private async writePayloadWithWritable(
handle: OPFSFileHandle,
response: Response,
): Promise<number> {
const writable = await handle.createWritable();
try {
if (response.body !== null) {
let nbytes = 0;
const reader = response.body.getReader();
try {
while (true) {
const { done, value } = await reader.read();
if (done) {
break;
}
await writable.write(value);
nbytes += value.byteLength;
}
} finally {
reader.releaseLock();
}
await writable.close();
return nbytes;
}
const payload = await response.arrayBuffer();
await writable.write(payload);
await writable.close();
return payload.byteLength;
} catch (err) {
try {
await writable.abort();
} catch {
// Ignore secondary errors during abort cleanup
}
throw err;
}
}
```
##########
web/src/opfs_store.ts:
##########
@@ -166,43 +283,178 @@ export class OPFSStore {
return this.directoryPromise;
}
- private async readMetadata(
+ private async readRecord(
fileHandle: OPFSFileHandle,
- ): Promise<OPFSStoreMetadata | undefined> {
+ ): Promise<OPFSStoreRecord | undefined> {
try {
const text = await (await fileHandle.getFile()).text();
const parsed = JSON.parse(text);
if (
parsed === undefined ||
parsed === null ||
typeof parsed !== "object" ||
- typeof parsed.url !== "string"
+ typeof parsed.url !== "string" ||
+ !Number.isSafeInteger(parsed.nbytes) ||
+ parsed.nbytes < 0
) {
- throw new Error("OPFSStore: invalid metadata format.");
+ return undefined;
}
- const metadata: OPFSStoreMetadata = {
+ const record: OPFSStoreRecord = {
url: parsed.url,
+ nbytes: parsed.nbytes,
};
if (typeof parsed.contentType === "string") {
- metadata.contentType = parsed.contentType;
+ record.contentType = parsed.contentType;
}
- return metadata;
+ return record;
} catch (err) {
- if (this.isNotFoundError(err)) {
- // Treat metadata disappearance between lookup and read as a cache miss
+ if (
+ OPFSStore.getErrorName(err) === "SyntaxError" ||
+ this.handleCacheMissStateError(err)
+ ) {
return undefined;
}
throw err;
}
}
- private async writeFile(
+ private getResponseInit(
+ record: OPFSStoreRecord,
+ ): ResponseInit | undefined {
+ return record.contentType !== undefined
+ ? { headers: { "content-type": record.contentType } }
+ : undefined;
+ }
+
+ private async writeRecord(
handle: OPFSFileHandle,
- data: Blob | BufferSource | string,
+ record: OPFSStoreRecord,
): Promise<void> {
const writable = await handle.createWritable();
- await writable.write(data);
+ await writable.write(new TextEncoder().encode(JSON.stringify(record)));
+ 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 hasExpectedPayloadSize(
+ entry: OPFSStoredEntry,
+ ): Promise<boolean> {
+ const blob = await entry.payloadHandle.getFile();
+ return blob.size === entry.record.nbytes;
+ }
Review Comment:
In `sync` mode, calling `entry.payloadHandle.getFile()` is an asynchronous
operation that creates a `Blob` object. For large model weights (often hundreds
of megabytes or gigabytes), creating a `Blob` just to check its size can
introduce non-trivial overhead and disk/memory mapping latency.
Since we are in `sync` mode, we can open a read-only sync access handle and
call `getSize()` to retrieve the file size. This is a metadata-only operation
that is extremely fast and avoids the overhead of creating a `Blob`.
```typescript
private async hasExpectedPayloadSize(
entry: OPFSStoredEntry,
): Promise<boolean> {
if (this.accessMode === "sync") {
const syncHandle = await
this.openSyncAccessHandle(entry.payloadHandle, "read-only");
if (syncHandle !== undefined) {
try {
return syncHandle.getSize() === entry.record.nbytes;
} finally {
syncHandle.close();
}
}
}
const blob = await entry.payloadHandle.getFile();
return blob.size === entry.record.nbytes;
}
```
##########
web/src/opfs_store.ts:
##########
@@ -166,43 +283,178 @@ export class OPFSStore {
return this.directoryPromise;
}
- private async readMetadata(
+ private async readRecord(
fileHandle: OPFSFileHandle,
- ): Promise<OPFSStoreMetadata | undefined> {
+ ): Promise<OPFSStoreRecord | undefined> {
try {
const text = await (await fileHandle.getFile()).text();
const parsed = JSON.parse(text);
if (
parsed === undefined ||
parsed === null ||
typeof parsed !== "object" ||
- typeof parsed.url !== "string"
+ typeof parsed.url !== "string" ||
+ !Number.isSafeInteger(parsed.nbytes) ||
+ parsed.nbytes < 0
) {
- throw new Error("OPFSStore: invalid metadata format.");
+ return undefined;
}
- const metadata: OPFSStoreMetadata = {
+ const record: OPFSStoreRecord = {
url: parsed.url,
+ nbytes: parsed.nbytes,
};
if (typeof parsed.contentType === "string") {
- metadata.contentType = parsed.contentType;
+ record.contentType = parsed.contentType;
}
- return metadata;
+ return record;
} catch (err) {
- if (this.isNotFoundError(err)) {
- // Treat metadata disappearance between lookup and read as a cache miss
+ if (
+ OPFSStore.getErrorName(err) === "SyntaxError" ||
+ this.handleCacheMissStateError(err)
+ ) {
return undefined;
}
throw err;
}
}
- private async writeFile(
+ private getResponseInit(
+ record: OPFSStoreRecord,
+ ): ResponseInit | undefined {
+ return record.contentType !== undefined
+ ? { headers: { "content-type": record.contentType } }
+ : undefined;
+ }
+
+ private async writeRecord(
handle: OPFSFileHandle,
- data: Blob | BufferSource | string,
+ record: OPFSStoreRecord,
): Promise<void> {
const writable = await handle.createWritable();
- await writable.write(data);
+ await writable.write(new TextEncoder().encode(JSON.stringify(record)));
+ await writable.close();
+ }
Review Comment:
Similar to `writePayloadWithWritable`, if `writable.write` throws an error,
the `writable` stream is not closed or aborted, leaving the record file locked.
We should wrap this in a `try...catch` block to abort the stream on failure.
```typescript
private async writeRecord(
handle: OPFSFileHandle,
record: OPFSStoreRecord,
): Promise<void> {
const writable = await handle.createWritable();
try {
await writable.write(new TextEncoder().encode(JSON.stringify(record)));
await writable.close();
} catch (err) {
try {
await writable.abort();
} catch {
// Ignore secondary errors during abort cleanup
}
throw err;
}
}
```
##########
web/src/opfs_store.ts:
##########
@@ -43,94 +51,193 @@ interface OPFSStorageManager {
getDirectory?: () => Promise<OPFSDirectoryHandle>;
}
-interface OPFSStoreMetadata {
+interface OPFSStoreRecord {
url: string;
+ nbytes: number;
contentType?: string;
}
+interface OPFSStoredEntry {
+ payloadHandle: OPFSFileHandle;
+ record: OPFSStoreRecord;
+}
+
+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 readonly requestedAccessMode: OPFSAccessMode;
+ private accessMode: OPFSEffectiveAccessMode;
private directoryPromise?: Promise<OPFSDirectoryHandle>;
- constructor(scope: string) {
+ constructor(scope: string, accessMode: OPFSAccessMode = "async") {
this.scope = scope;
+ this.requestedAccessMode = accessMode;
+ 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;
+ try {
+ const entry = await this.getStoredEntry(url);
+ if (entry === undefined) {
+ return false;
+ }
+ return this.hasExpectedPayloadSize(entry);
+ } catch (err) {
+ if (this.handleCacheMissStateError(err)) {
+ return false;
+ }
+ throw err;
+ }
}
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) {
- return undefined;
+ try {
+ const entry = await this.getStoredEntry(url);
+ if (entry === undefined) {
+ return undefined;
+ }
+ if (this.accessMode === "async") {
+ const blob = await entry.payloadHandle.getFile();
+ if (blob.size !== entry.record.nbytes) {
+ return undefined;
+ }
+ return new Response(blob, this.getResponseInit(entry.record));
+ }
+ const payload = await this.readPayload(entry.payloadHandle);
+ if (payload.byteLength !== entry.record.nbytes) {
+ return undefined;
+ }
+ return new Response(payload, this.getResponseInit(entry.record));
+ } catch (err) {
+ if (this.handleCacheMissStateError(err)) {
+ return undefined;
+ }
+ throw err;
}
- 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.");
+ }
Review Comment:
In `sync` mode, `read(url)` eagerly reads the entire payload into an
`ArrayBuffer` in JS memory via `readPayload` before wrapping it in a
`Response`. Eagerly loading large files (like model weights) into JS memory can
cause high memory pressure or out-of-memory (OOM) errors in the worker.
Since `read(url)` returns a `Response` object, we should always wrap a lazy
`Blob` (obtained via `getFile()`) instead of an eager `ArrayBuffer`. This
allows the browser to handle the file content lazily or stream it, which is
much more memory-efficient. Callers who specifically need an `ArrayBuffer`
should use `readArrayBuffer(url)` instead, which is already optimized to use
the sync access handle directly.
This also simplifies the `read` method by removing the need to branch on
`this.accessMode`.
```typescript
async read(url: string): Promise<Response | undefined> {
try {
const entry = await this.getStoredEntry(url);
if (entry === undefined) {
return undefined;
}
const blob = await entry.payloadHandle.getFile();
if (blob.size !== entry.record.nbytes) {
return undefined;
}
return new Response(blob, this.getResponseInit(entry.record));
} catch (err) {
if (this.handleCacheMissStateError(err)) {
return undefined;
}
throw err;
}
}
```
--
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]
