Improve `Actor` typings #3225

HarelM · 2023-10-16T15:03:22Z

Launch Checklist

This mainly type changes (a few minor code changes).
I needed a break from the angular/webpack/babel stuff.

@msbarry can you take a look and suggest what can be done better?
Going forward I would like every message type to have a data type associated with it so that when you send a message you know what data you send with it, but that can be done as a later step.

HarelM · 2023-10-16T15:06:43Z

src/util/actor.ts

@@ -69,40 +96,35 @@ export class Actor {
        if (callback) {
            this.callbacks[id] = callback;
        }
-        const buffers: Array<Transferable> = isSafari(this.globalScope) ? undefined : [];
-        this.target.postMessage({
+


I have removed the isSafari part here as MDN signature is not compatible with this and I think it's not needed, but it needs to be verified with Safari I guess...

Wait don't we actually still want to keep this? It makes message transfers faster in all non-safari browsers by listing out the objects where ownership can be transferred to the other worker without copying the contents

https://developer.mozilla.org/en-US/docs/Web/API/Worker/postMessage#transfer

From what I can tell here https://caniuse.com/mdn-api_window_postmessage_transfer_parameter it is well supported - I wonder why it was disabled on safari? Maybe it had a negative performance impact?

I couldn't make typescript happy with this parameter, I'll try again now, thanks for the link!

I understand why I removed this parameter - the worker API for postMessage is different than the window API for postMessage. The actor servers both window (main thread) and the web worker.
I'm not sure what is the outcome of sending an empty array when sending a message from the main thread.
Maybe I can use the transfer parameter of options as it seems both are supporting it.
https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage

This is the original issue.
mapbox/mapbox-gl-js#8771
Seems that I removed all the transferable object by mistake, I need to revert this.
According to the above last comment the issue in safari is now fixed and it should be safe to add the transferable objects again.

msbarry · 2023-10-18T00:50:16Z

src/util/actor.ts

+export type MessageType = '<response>' | '<cancel>' |
+'geojson.getClusterExpansionZoom' | 'geojson.getClusterChildren' | 'geojson.getClusterLeaves' | 'geojson.loadData' |
+'removeSource' | 'loadWorkerSource' | 'loadDEMTile' | 'removeDEMTile' |
+'removeTile' | 'reloadTile' | 'abortTile' | 'loadTile' | 'getTile' |
+'getGlyphs' | 'getImages' | 'setImages' |
+'syncRTLPluginState' | 'setReferrer' | 'setLayers' | 'updateLayers';


I think you can also do keyof Worker here instead of enumerating these to get them automatically from the methods defined in worker.ts.

The geojson.* ones wouldn't come for free, but we could probably either do some fancy typescript work to create them automatically from a list of WorkerSource types, or just make explicit methods on worker geojsonGetClusterLeaves and getjsonLoadData since there's only 2 of them.

I agree, but this is not the way I would like to move forward, I don't think that a message type should be a method name, it's not refactoring-safe, what I would like to do going forward is "register" an even type with a specific method, much like pub-sub and introduce promises to this entire mess.
I think this would be the first step toward reducing the number of callbacks from the code.
But I want this initial step to have more clarity on how to change things and how to proceed.

The geojson part seems like a hack I'll be removing once this will be refactored to something that is easy to read and maintain.

It might help to see some pseudocode of the end state you're looking for. Is it something like:

// in shared code: define a message object/type class LoadDemTileRequest { ... } // in worker: register handler actor.register(LoadDemTileRequest, demWorker.loadTile); // in main: send using shared message object/type actor.send(new LoadDemTileRequest(...))

?

I think it's possible to get the existing "method of worker" approach to work with promises, but you've got a good point that we've got a chance to rethink this now so don't need to feel bound by the old implementation.

Yes, something similar to what you wrote, I still don't have a good picture of how this will look as I need to understand most of the places this is used and how to best define it.
For example I leaned that Style class is registering itself to provide getGlyphs method to the workers. It took me too long to figure out why and where the registration occures and how this works.
It shouldn't be that complicated...
So I need to map the events and see how I can slowly progress and to it bit by bit...

msbarry · 2023-10-18T00:56:30Z

src/util/actor.ts

@@ -55,7 +82,7 @@ export class Actor {
     * @param targetMapId - A particular mapId to which to send this message.
     */
    send(
-        type: string,
+        type: MessageType,
        data: unknown,


Is it in scope for this PR to make data argument typed so you know you are passing the right data into it? Here's what I landed on in maplibre-contour:

https://github.com/onthegomap/maplibre-contour/blob/6abe4628329227337ae536c9b3eb6dc9658560c2/src/actor.ts#L107-L117

Something like:

type KeysOfType<T, V> = { [K in keyof T]: T[K] extends V ? K : never; }[keyof T]; export type MessageType = KeysOfType<WorkerType, (a: any, b: any, c: WorkerTileCallback) => void>; send( type: MessageType, data: Parameters<WorkerType[MessageType]>[1], callback?: Function | null, targetMapId?: string | null, mustQueue: boolean = false ): Cancelable {

I think this is OK for this PR, I'm thinking about creating a sendAsync method that will get a message, each message will have a type and a payload that is specific to that message, this way you know what you are passing and what you are expecting in return.
This will return a promise with a single data object that will have all the relevant data.
It will be a slow process to fix all the places, but once done, I'll simply remove this send method.

That makes sense - the typing gets pretty gnarly to make that work, makes sense to set up the typing we want on the new clean-slate implementation and clean things up as we migrate to it.

HarelM · 2023-10-18T06:44:27Z

There's also this which might reduce the need to write any of this:
https://github.com/GoogleChromeLabs/comlink

HarelM · 2023-10-18T08:19:37Z

Doesn't look like comlink will be very helpful due to the spaghetti of a code that currently exist around the actor code.
I'll need to simplify it, refactor it and only then I might be able to use comlink, which in that point would probably not worth the effort...

msbarry · 2023-10-18T09:55:43Z

There's also this which might reduce the need to write any of this: https://github.com/GoogleChromeLabs/comlink

I'd also hesitate to to remove the task queue/throttled invoker in actor. I know a lot of work has went into optimizing how those work to make the map as responsive as possible with lots of messages going back and forth. It seems safe to swap-out the message types and how they are dispatched though.

codecov-commenter · 2023-10-18T13:04:27Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (748c0b4) 75.23% compared to head (214b783) 75.23%.
Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3225   +/-   ##
=======================================
  Coverage   75.23%   75.23%           
=======================================
  Files         241      241           
  Lines       19253    19255    +2     
  Branches     4339     4337    -2     
=======================================
+ Hits        14485    14487    +2     
  Misses       4768     4768

Files	Coverage Δ
src/source/worker.ts	`76.99% <100.00%> (ø)`
src/util/actor.ts	`80.85% <100.00%> (+0.41%)`	⬆️
src/util/dispatcher.ts	`100.00% <100.00%> (ø)`
src/util/web_worker.ts	`100.00% <ø> (ø)`
src/util/web_worker_transfer.ts	`88.09% <100.00%> (ø)`
src/util/worker_pool.ts	`92.30% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

HarelM · 2023-10-18T13:42:36Z

OK, I think the code now is a lot more close to what it used to be.
I removed the check for Safari as it seems like the memory issue was resolved there.
So mainly types additions.

initial commit to improve typings of actor

4e835f5

HarelM commented Oct 16, 2023

View reviewed changes

Fix lint

133c490

msbarry reviewed Oct 18, 2023

View reviewed changes

HarelM requested review from neodescis and birkskyum October 18, 2023 08:17

birkskyum approved these changes Oct 18, 2023

View reviewed changes

Bring back empry transfer

0cae0d0

HarelM added 2 commits October 18, 2023 16:05

Align with previous code better.

7c9f036

Bring back transferable usage

214b783

HarelM enabled auto-merge (squash) October 18, 2023 13:44

HarelM merged commit ccccef1 into main Oct 18, 2023

HarelM deleted the improve-actor-types branch October 18, 2023 13:54

rejas mentioned this pull request Oct 25, 2023

Release 3.5.2 #3271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `Actor` typings #3225

Improve `Actor` typings #3225

HarelM commented Oct 16, 2023 •

edited

Loading

HarelM Oct 16, 2023

msbarry Oct 18, 2023 •

edited

Loading

HarelM Oct 18, 2023

HarelM Oct 18, 2023

HarelM Oct 18, 2023

msbarry Oct 18, 2023

HarelM Oct 18, 2023

HarelM Oct 18, 2023

msbarry Oct 18, 2023

HarelM Oct 18, 2023

msbarry Oct 18, 2023 •

edited

Loading

HarelM Oct 18, 2023

msbarry Oct 18, 2023

HarelM commented Oct 18, 2023

HarelM commented Oct 18, 2023

msbarry commented Oct 18, 2023

codecov-commenter commented Oct 18, 2023 •

edited

Loading

HarelM commented Oct 18, 2023

Improve Actor typings #3225

Improve Actor typings #3225

Conversation

HarelM commented Oct 16, 2023 • edited Loading

Launch Checklist

Choose a reason for hiding this comment

msbarry Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msbarry Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarelM commented Oct 18, 2023

HarelM commented Oct 18, 2023

msbarry commented Oct 18, 2023

codecov-commenter commented Oct 18, 2023 • edited Loading

Codecov Report

HarelM commented Oct 18, 2023

Improve `Actor` typings #3225

Improve `Actor` typings #3225

HarelM commented Oct 16, 2023 •

edited

Loading

msbarry Oct 18, 2023 •

edited

Loading

msbarry Oct 18, 2023 •

edited

Loading

codecov-commenter commented Oct 18, 2023 •

edited

Loading