Skip to content

Commit

Permalink
chore(cu): ensure ArrayBuffer, and not a View, is transferred between…
Browse files Browse the repository at this point in the history
… threads to prevent truncation
  • Loading branch information
TillaTheHun0 committed Jul 22, 2024
1 parent aff74ae commit f7ce814
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 7 deletions.
19 changes: 18 additions & 1 deletion servers/cu/src/domain/client/ao-module.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { fromPromise, of, Rejected, Resolved } from 'hyper-async'
import { always, applySpec, defaultTo, evolve, head, prop } from 'ramda'
import { z } from 'zod'

import { arrayBufferFromMaybeView } from '../utils.js'
import { moduleSchema } from '../model.js'
import { MODULES_TABLE } from './sqlite.js'
import { timer } from './metrics.js'
Expand Down Expand Up @@ -163,7 +164,23 @@ export function evaluatorWith ({ evaluate, loadWasmModule }) {
stopTimer()
}

options = { transfer: [ArrayBuffer.isView(args.Memory) ? args.Memory.buffer : args.Memory] }
/**
* If Memory is sufficiently large, transferring the View somehow
* causes the underlying ArrayBuffer to be truncated. This truncation
* does not occur when instead the underlying ArrayBuffer is transferred,
* directly.
*
* So we always ensure the Memory transferred to the worker thread
* is the actual ArrayBuffer, and not a View.
*
* (the same is done in the opposite direction in the worker thread)
*
* TODO: maybe AoLoader should be made to return the underlying ArrayBuffer
* as Memory, instead of a View?
*/
args.Memory = arrayBufferFromMaybeView(args.Memory)

options = { transfer: [args.Memory] }
}

args.streamId = streamId
Expand Down
4 changes: 4 additions & 0 deletions servers/cu/src/domain/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -426,3 +426,7 @@ export const maybeParseInt = pipe(
mapBadData,
(val) => val ? parseInt(val) : undefined
)

export const arrayBufferFromMaybeView = (maybeView) => ArrayBuffer.isView(maybeView)
? maybeView.buffer
: maybeView // assumes an ArrayBuffer. TODO: maybe an additional check
20 changes: 19 additions & 1 deletion servers/cu/src/domain/worker/evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,25 @@ export function evaluateWith ({
logger('Evaluating message "%s" to process "%s"', name, processId)
return wasmInstance
})
.chain(fromPromise(async (wasmInstance) => wasmInstance(Memory, message, AoGlobal)))
.chain(fromPromise(async (wasmInstance) =>
/**
* AoLoader requires Memory to be a View, so that it can set the WebAssembly.Instance
* memory.
*
* DataView.set is used internally in AoLoader to "reload" the Memory
* into a WebAssembly.Instance. set() claims to
* "intelligently copy the source range of the buffer to the destination range
* [if they share the same underlying ArrayBuffer]" but doesn't go into detail about what
* "intelligent" actually means.
*
* It seems to imply that only diffs will be copied, if any at all. So for the same
* exact underlying ArrayBuffer with no diffs, I would _hope_ that is little to no
* performance penalty, but we should verify
*
* TODO: check if any performance implications in using set() within AoLoaderrs
*/
wasmInstance(ArrayBuffer.isView(Memory) ? Memory : new Uint8Array(Memory), message, AoGlobal)
))
.bichain(
/**
* Map thrown error to a result.error. In this way, the Worker should _never_
Expand Down
17 changes: 12 additions & 5 deletions servers/cu/src/domain/worker/evaluator/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { fetch, setGlobalDispatcher, Agent } from 'undici'
import { worker, Transfer } from 'workerpool'

import { createLogger } from '../../logger.js'
import { arrayBufferFromMaybeView } from '../../utils.js'

import { createApis } from './main.js'

Expand Down Expand Up @@ -50,12 +51,18 @@ worker({
if (!output.Memory) return output

/**
* We make sure to only transfer the underlying ArrayBuffer
* back to the main thread.
* If Memory is sufficiently large, transferring the View somehow
* causes the underlying ArrayBuffer to be truncated. This truncation
* does not occur when instead the underlying ArrayBuffer is transferred,
* directly.
*
* So we always ensure the Memory transferred back to the main thread
* is the actual ArrayBuffer, and not a View.
*
* TODO: maybe AoLoader should be made to return the underlying ArrayBuffer
* as Memory, instead of a View?
*/
output.Memory = ArrayBuffer.isView(output.Memory)
? output.Memory.buffer
: output.Memory
output.Memory = arrayBufferFromMaybeView(output.Memory)

return new Transfer(output, [output.Memory])
})
Expand Down

0 comments on commit f7ce814

Please sign in to comment.