-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove offscreen and parallelize transaction building #1769
base: main
Are you sure you want to change the base?
Conversation
|
3027e44
to
afd1319
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@turbocrime the block processor component still needs to be migrated within the offscreen client – what would that generally look like and how significant of a lift would it be?
|
||
let active = 0; | ||
|
||
const activateOffscreen = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: for comparison sake, here's the control flow of the previous implementation:
-
The offscreenClient object was exported, which includes the buildActions method,
-
Inside optimisticBuild, offscreenClient.buildActions was invoked to initiate the offscreen document lifecycle.
-
Within buildActions, an offscreen document was created and managed using
chrome.offscreen.createDocument
. -
Collection of promises and request messages was generated and transmitted to offscreen workers via chrome.runtime.sendMessage.
-
The extension listens for these requests using a chrome event listener chrome.runtime.onMessage.addListener.
-
This listener triggers an async IIFE (Immediately Invoked Function Expression) that handles the async action-building process.
-
The spawnActionBuildWorker function was then called, spawning a web worker for the typescript file (
wasm-build-action.ts
) using the standard Worker API. -
Once the worker event listeners are set up, executeWorker was called to execute the actual wasm module.
* Build actions in parallel by launching a worker for each action in the plan. | ||
* @returns An individually-promised list of build results. | ||
*/ | ||
export const launchActionWorkers = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: for external reviewers, this delineates some of the design considerations based on offline conversations.
The service worker is a single-threaded runtime, and objective is to lift the trial decryption to operate in an offscreen context, preventing it from blocking the service worker. The possible approaches to achieve this are:
- target standard dom
- require offscreen client to be provided
target standard dom
previously, the services package was responsible for managing the offscreen state by relying on chrome's offscreen API. The services package was refactored to abandon the offscreen API entirely, instead using standard DOM (built-in Worker API) to manage workers.
This approach is more general and works outside of chrome extension limitations. If standard DOM is available, the Worker API is used directly. If not, a polyfill simulates the same environment (e.g., using offscreen workers). Services and block processors can directly create and manage workers using a standardized API, and there’s no dependency on chrome’s runtime or messaging.
require offscreen client to be provided (via context, or constructor)
this approach abstracts the worker-launching mechanism so that services don’t manage workers directly. Instead, workers are launched through a generalized interface, which could be either a standard DOM worker to a chrome offscreen worker. The services packages would be agnostic to the specific worker implementation and lifecycle.
This would require moving the offscreen client into the handler context for services and pass as a parameter for the block processor on initialization, so they can use the same interface to launch and manage offscreen.
In comparison to the first approach, this serves as a generalized worker launching interface provided externally to launch workers, where you're calling a generic interface method rather than a worker constructor (standard DOM).
We opted for the first approach, as the difference between the two is minimal in terms of the actual implementation effort.
actionPlanIndex, | ||
}; | ||
|
||
const actionWorker = new Worker(new URL('./build-action-worker.js', import.meta.url)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: previously we were spawning the worker in the extension's offscreen handler, and now it's happening directly in the services package.
Based on offline discussion, a generic offscreen utility called OffscreenWorker now manages the lifecycle of the offscreen document. A polyfill was created where new Worker
references the global Worker
variable. So when the service worker sets globalThis.Worker = OffscreenWorker, all Worker
references in the script now point to OffscreenWorker
. Each new Worker
call creates an instance of OffscreenWorker
, which communicates via a dedicated port to an actual Worker
running in the offscreen document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also benchmark the web worker performance to identify any potential regressions in the proving time, though there shouldn't be any.
has there been extensive manual testing that the optimistic proving mechanism doesn't somehow break OffscreenWorker which manages the lifecycle of the offscreen document? |
references #1787
this PR removes the 'offscreen' logic and thus all chrome runtime calls from services. parallel builds now call
new Worker
directly. the package now uses DOM lib in tsconfig.keys package now more correctly supports consumers attempting to identify asset paths.
'internal message' types are now removed from the types package.