Open lordofthelake opened 4 years ago
We can’t really make it asynchronous otherwise that’s not a react renderer anymore. Additionally, asynchronous code in Sketch brings quite a lot of issues.
Let's go with 1.. It's fine to hang Sketch while rendering. It'd be actually pretty bad if the user starts to interact with Sketch while we are modifying the document.
I guess we can use https://github.com/ForbesLindesay/sync-request#readme on the node side and keep using NSData on the sketch side
We can’t really make it asynchronous otherwise render would be asynchronous and that’s not a react renderer anymore.
Well, I would argue that it isn't a React renderer now either, because normally during rendering there wouldn't be any asset fetching – that would be handled asynchronously by the browser/components (possibly causing re-renders and reconciliation as the state changes). So, the "correct" way of doing it would be to do multiple renders as the data comes through, but it's obviously quite impractical, given that we treat Sketch a "static" medium.
In fact, the only other React renderer that I know that works with a "static" medium and that fetches assets during the render phase, react-pdf
, has an asynchronous render method.
And asynchronous code in Sketch brings quite a lot of issues.
What kind? The tree would still be emitted all at once when all the promises resolve, so from a Sketch perspective, it shouldn't be any different from the data fetching example in the docs.
I can’t remember now, is there a synchronous http api in Node? If so i could try to polyfill in Sketch
Not that I know of. http
is stream-based and has a quite extensive surface, so I don't think it would be the best candidate for polyfilling either. I guess I could probably figure out some kind of hack with mutexes or something, but it's really fitting a square peg in a round hole with Node, so anything that comes out of that will be the ugliest.
What kind?
As soon as you have async code, hot reloading is breaking and you have memory issues except if you know exactly how to clean every thing up and how cocoascript is working internally (which even after working 2 years with, I wouldn't say I know exactly how it's working).
it shouldn't be any different from the data fetching example in the docs
The big difference is that it's user code and we aren't responsible for it.
anything that comes out of that will be the ugliest
https://github.com/ForbesLindesay/sync-request#readme doesn't look amazing but it probably does the job.
Alright I'll give a shot to sync-request
and see what happens.
Another solution (if you really want to go async) that I'd prefer to an async render
method would be to render a rectangle without the image, do the request asynchronously and then add a fill to the rectangle once we have the image. It would be kind of a "rerender" when we get the data.
Then we have renderToJSON
that can be a promise (I don't mind as much since I don't think it's used by anybody else but Lona).
But I think it's a lot more complicated to implement than just a synchronous API
Another solution (if you really want to go async) that I'd prefer to an async
render
method would be to render a rectangle without the image, do the request asynchronously and then add a fill to the rectangle once we have the image. It would be kind of a "rerender" when we get the data.Then we have
renderToJSON
that can be a promise (I don't mind as much since I don't think it's used by anybody else but Lona).But I think it's a lot more complicated to implement than just a synchronous API
This would still need asynchronicity somewhere though to track the requests, no? You would build one tree, collect the images to load in a pool in the meantime, emit the tree, wait for the pool to load and then emit the tree again. The user would still need a notification somewhere about when the process is complete (either a callback or returning a Promise), if they need to do something that depends on the render being complete and "settled" (or at least to get a notification on whether an error has occurred).
Or you don't you care about render()
returning only when the layout has "settled"? Would in your vision a RedBox
in Sketch suffice as the error reporting system and we don't care about the fact that after render()
returns there might be more re-renders in background?
I mostly want to understand how in your vision the external API should look like, because I think that internally the problem is solvable, because it's already a "two phase render": first the complete tree gets built, then the complete tree gets emitted. The tree building would be async, while the emitting would be sync.
It's still single-threaded and I don't see too many native objects crossing method boundaries, so I'm not hugely worried about memory leaks. In the worst-case scenario, at the end of every "render phase", we can always clean everything up (something that I would do for Yoga nodes BTW, that need to be freed explicitly).
This would still need asynchronicity somewhere though to track the requests, no?
Yeah, that a solution I was thinking about in case you really want it to be async.
Or you don't you care about render() returning only when the layout has "settled"?
I don't care for the case of render
indeed.
I don't think we need a complete rerender. Each image renderer tracks the id of the shape it creates, emit the rectangle shape without the image fill, fetch the data and when the data arrives, imperatively add a fill to the shape.
too many native objects crossing method boundaries
fetch
would be one.
I'm not hugely worried about memory leaks
You'd be surprised 😄
we can always clean everything up
That's really not that simple. It's not V8. JavaScriptCore isn't made to execute asynchronous code so it's Sketch handling it. And it's not straightforward to say the least.
something that I would do for Yoga nodes BTW, that need to be freed explicitly
Again, not as simple (I saw your comment on the other issue). The fact that something isn't freed means that the javascript vm is keeping a reference to a native object (eg. not a yoga node which are pure JS and don't matter) even after the event loop is empty. That's the reason why the vm can't be garbage collected by Sketch.
Quick check to understand whether my mental model of the execution environment is correct:
alloc().init()
, but I don't see any deallocation or equivalent of an @autoreleasepool
do we have to rely on the JS garbage collection?
yes
I don't see any deallocation
That's because of ARC. There is no deallocation of an obj-c object anymore, only of C pointers (but I don't think there should be any?)
Are we running on a similar version of JSC to the one powering Safari or React Native? Just without the browser/native APIs? How old of a version are we talking about here?
JSC is a system API, so you use the version of the os. It is a JS engine, so no browser API indeed (as they aren't part of emacscript).
So if you allocate them, you have also to free them, otherwise the memory in ASM will never be released. Am I missing something?
We are using the JS backend. ASM doesn't work on JSC.
JSC is a system API, so you use the version of the os. It is a JS engine, so no browser API indeed (as they aren't part of emacscript).
So on recent OS versions should be pretty up-to-date.. at least as up-to-date as the version of Safari that shipped with the OS, right?
So if you allocate them, you have also to free them, otherwise the memory in ASM will never be released. Am I missing something?
We are using the JS backend. ASM doesn't work on JSC.
The JS backend is still C++ -> emscriptem -> asm.js right? That's at least what I see on the yoga repo. It'n not WebAssembly, but still the virtual memory is simulated through one big typed array buffer. So, unless I missed something, we still need to manage memory for them and deallocate the nodes once we don't use them anymore, otherwise we're potentially filling the "ASM.js virtual memory buffer". Documentation is pretty much non-existent, but I see that they expose .destroy()
methods in the JS interfaces, that I suppose are for deallocation in "simulated native" land.
at least as up-to-date as the version of Safari that shipped with the OS
yep yep
I see that they expose .destroy() methods in the JS interfaces
I'm guessing it's just to be compatible with the native bindings in Node? Even when using typed array, you don't have access to the garbage collector in JS, do you? At the end of the event loop, the VM should free any JS made object. It doesn't free some of the native stuff because cocoascript is doing nasty stuff (like storing privates on objects, etc.) but that doesn't apply to pure JS objects. But I guess it's worth a try to free the yoga nodes nevertheless
A quick update on this: I did a quick test spike yesterday making everything async and so far, from a quick test, it seems to just work fine. I'll clean up some stuff and open a draft PR, so that you can start taking a look at how something like this would look like and we can take it from there.
I'll test it on something bigger like antd-design later, so that we can check how leaks look like and I'll see if I can plug those as well.
Since we have available the JSC version of the Safari that shipped with the OS, assuming that we would have as minimum requirement High Sierra (following Sketch support) for a future breaking change, we would actually be targeting Safari 11 in terms of JS support. async
/await
works fine there, so no need to transpile down to ES5 and use regenerator-runtime.
I'm guessing it's just to be compatible with the native bindings in Node? Even when using typed array, you don't have access to the garbage collector in JS, do you? At the end of the event loop, the VM should free any JS made object.
Yoga is written in C++, so no GC exists from their perspective. They just malloc()
memory in a virtual address space, that in the case of asm.js is emulated with a big typed array of bytes. To nullify those entries in the array, they need to call somewhere a free()
, otherwise the bytes will stay there and they'll need to allocate more memory for new objects in their simulated address space (i.e. filling more bytes in the typed array/virtual memory simulation). The JS GC can't do much about it either, because from where it stands, it just sees an array full of bytes. The JS wrapper objects can be GC'd when they go out of scope, but since there is no concept of a "destructor" in JS, they can't be notified to free up the "simulated native" memory bytes in the virtual memory array that they reference. Hence I think that we have to do this for them, otherwise with a lot of nodes (like in antd), we'll just keep allocating memory (that we can't reference anymore either, because we don't have the corresponding Yoga.Node
s anymore).
A quick and dirty hack could be also to see if there is any way to "unload" and "reload" Yoga (the equivalent of killing the process and respawning it). Since re-renders are an infrequent business, just "shutting down" Yoga between re-renders might be a good option (assuming that any binding for doing that exists).
no need to transpile down to ES5 and use regenerator-runtime
Any reason not to? We already have a build step so I'd rather be more compatible than not enough. Can't remember the Sketch version we support but if it's 50+, that's El Capitan
The JS wrapper objects can be GC'd when they go out of scope, but since there is no concept of a "destructor" in JS, they can't be notified to free up the "simulated native" memory bytes in the virtual memory array that they reference.
Indeed, you can't manually free memory in JSC, as soon as the event loop is empty, everything* is garbage collected by the system (* except the stuff that are manually tracked on the obj-c side). ASM.js is a subset of JS, so I don't see how a simulated address space can change that. Sure you have an opaque typed array but that shouldn't prevent the garbage collection to run. As long as you aren't using WebAassembly (and we aren't), you still just have a normal array. You don't have to set all the variables you define to null
at the end of every scripts.
no need to transpile down to ES5 and use regenerator-runtime
Any reason not to? We already have a build step so I'd rather be more compatible than not enough. Can't remember the Sketch version we support but if it's 50+, that's El Capitan
It can work with regenerator-runtime, but then there's a polyfill to inject with skpm (and one more runtime dependency). I skipped it for now, as it was quicker not to add other forks in the mix and just transpile to es2017, but I can re-add it without too much trouble. Safari 11 is available on El Capitan anyway. If JSC gets updated with Safari, it would still have support.
Indeed, you can't manually free memory in JSC, as soon as the event loop is empty, everything* is garbage collected by the system (* except the stuff that are manually tracked on the obj-c side). ASM.js is a subset of JS, so I don't see how a simulated address space can change that. Sure you have an opaque typed array but that shouldn't prevent the garbage collection to run. As long as you aren't using WebAassembly (and we aren't), you still just have a normal array. You don't have to set all the variables you define to
null
at the end of every scripts.
Does the JS VM get shut down between plug-ins invocations or is it just left idle? Because that array is a global variable, so even if the event loop is empty, it won't be GC'd until the end of the process.
Does the JS VM get shut down between plug-ins invocations or is it just left idle? Because that array is a global variable, so even if the event loop is empty, it won't be GC'd until the end of the process.
It gets shut down if there isn't any native stuff keeping a reference to it. A new one is created every time. That's why there is a memory leak. A native object isn't freed and so the VM is kept around while it shouldn't.
You don't have to free every global variables, that'd be a nightmare 😄
Does the JS VM get shut down between plug-ins invocations or is it just left idle? Because that array is a global variable, so even if the event loop is empty, it won't be GC'd until the end of the process.
It gets shut down if there isn't any native stuff keeping a reference to it. A new one is created every time. That's why there is a memory leak. A native object isn't freed and so the VM is kept around while it shouldn't.
So in plugins there is no persistent state between invocations? That kinda sucks :sweat_smile:
You don't have to free every global variables, that'd be a nightmare 😄
Well, it would be just like a SPA or a long-running server process. It's not so bad :smile:
in plugins there is no persistent state between invocations?
You can keep some state between invocations: https://developer.sketch.com/reference/api/#get-a-session-variable
it would be just like a SPA or a long-running server process
You can do that for sure in Sketch https://developer.sketch.com/reference/api/#async, and that's when bugs start to appear: if you try to access stuff that are deallocated, or from another thread, Sketch will just crash. You don't have the safety of JS when you do stuff that you aren't supposed to in JSC.
Ah ok, I'm starting to see where this is heading. The sketch/async
APIs give you access to real parallelism, that's not just good the ole' single-threaded event loop with async IO. Is the fetch
polyfill thread-safe, as in posting back the callbacks in the main thread loop?
no sketch/async
just tells Sketch to keep the script around (eg. not to garbage collect the VM when the event loop is empty). It's then up to you to use native APIs to do async stuff. So fetch
uses NSURLRequest
for example. But it should "safe".
But keeping the VM around breaks the hot reloading (the code in the VM isn't updated when calling the script again), and it often brings a lot of issues that you can work around if you know how it works but as you can see, it's really not straightforward. So if we can avoid it, I'd rather do that.
Well, in this case, I wouldn't be touching sketch/async
at all.
What I'm working with here is just the JS "garden variety" of asynchronicity, just changing the interfaces to return Promises. I have no intention of adding real parallelism to the mix, because that's way beyond what would be useful for my purpose and a whole mess I really don't want to deal with :smile:
So since my breach into "native world" would be limited to fetch
and the fs
polyfill, all the existing assumptions should still hold, right? I'm still confined to one, single-threaded, VM and when the render "settles" (aka the event loop drains), it should all still go away.
It can work with regenerator-runtime, but then there's a polyfill to inject with skpm (and one more runtime dependency). I skipped it for now, as it was quicker not to add other forks in the mix and just transpile to es2017, but I can re-add it without too much trouble.
wait regenerator-runtime
is a babel thing, I'd be surprise if TS generates code that can't be run as is. Pretty sure it includes the polyfill in the file.
wait
regenerator-runtime
is a babel thing, I'd be surprise if TS generates code that can't be run as is. Pretty sure it includes the polyfill in the file.
I tried it and it was failing at runtime because "regeneratorRuntime is not defined"
and indeed there was no import injected. This is a minor problem anyway, it's just a matter of either changing TS config or passing it through @babel/plugin-transform-runtime
at the end. I will figure this bit out after taking care of the rest, it shouldn't be a big deal.
but there isn't any babel config when compiling typescript. npm run build
is just tsc
. You can't add babel plugins to a TS config.
Maybe you are talking about the example you are using to test, without building react-sketchapp?
but there isn't any babel config when compiling typescript.
npm run build
is justtsc
. You can't add babel plugins to a TS config. Maybe you are talking about the example you are using to test, without building react-sketchapp?
No I know. regenerator-runtime
is not a babel-specific thing. It's also how TS (and everyone) transpiles generators and async/await for systems that don't have the support. The difference with Babel seems to be that TS injects the reference, but not the requirement. I'll investigate why.
If tsc
can't inject a polyfill, there are four solutions to the problem:
@babel/plugin-transform-runtime
to add the polyfills locally@babel/preset-typescript
and use tsc
only for file definitionsregenerator-runtime
manually in the filesProvidePlugin
One way or another this is a minor problem, I just need to fiddle with the tooling and I'll get it right.
Pretty sure that's not true. I just tried to compile
export const test = async (hello: string) => {
return await Promise.resolve(hello);
};
and it generates (with the es5
target)
"use strict";
var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
return new (P || (P = Promise))(function (resolve, reject) {
function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); }
step((generator = generator.apply(thisArg, _arguments || [])).next());
});
};
var __generator = (this && this.__generator) || function (thisArg, body) {
var _ = { label: 0, sent: function() { if (t[0] & 1) throw t[1]; return t[1]; }, trys: [], ops: [] }, f, y, t, g;
return g = { next: verb(0), "throw": verb(1), "return": verb(2) }, typeof Symbol === "function" && (g[Symbol.iterator] = function() { return this; }), g;
function verb(n) { return function (v) { return step([n, v]); }; }
function step(op) {
if (f) throw new TypeError("Generator is already executing.");
while (_) try {
if (f = 1, y && (t = op[0] & 2 ? y["return"] : op[0] ? y["throw"] || ((t = y["return"]) && t.call(y), 0) : y.next) && !(t = t.call(y, op[1])).done) return t;
if (y = 0, t) op = [op[0] & 2, t.value];
switch (op[0]) {
case 0: case 1: t = op; break;
case 4: _.label++; return { value: op[1], done: false };
case 5: _.label++; y = op[1]; op = [0]; continue;
case 7: op = _.ops.pop(); _.trys.pop(); continue;
default:
if (!(t = _.trys, t = t.length > 0 && t[t.length - 1]) && (op[0] === 6 || op[0] === 2)) { _ = 0; continue; }
if (op[0] === 3 && (!t || (op[1] > t[0] && op[1] < t[3]))) { _.label = op[1]; break; }
if (op[0] === 6 && _.label < t[1]) { _.label = t[1]; t = op; break; }
if (t && _.label < t[2]) { _.label = t[2]; _.ops.push(op); break; }
if (t[2]) _.ops.pop();
_.trys.pop(); continue;
}
op = body.call(thisArg, _);
} catch (e) { op = [6, e]; y = 0; } finally { f = t = 0; }
if (op[0] & 5) throw op[1]; return { value: op[0] ? op[1] : void 0, done: true };
}
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.test = function (hello) { return __awaiter(void 0, void 0, void 0, function () {
return __generator(this, function (_a) {
switch (_a.label) {
case 0: return [4 /*yield*/, Promise.resolve(hello)];
case 1: return [2 /*return*/, _a.sent()];
}
});
}); };
which does include the async/await polyfill
I'll investigate further where the error is coming from and fix it. Again, it's really not a problem. I just sidestepped it yesterday because I wanted to have something running without fiddling with the tools.
As we discussed in the PR over node-sketch-bridge, I'm working on writing a "pure" implementation of
makeImageDataFromUrl()
, usingfetch
and@skpm/fs
.There is one major problem implementing it in this way though:
fetch
is asynchronous, while the whole rendering operation is synchronous.Now, I see two ways around this:
NSData
around, and keep everything sync. Not surprisingly, NSData documentation has a big fat warning about not using it for network requests, though, because it can potentially hang the whole thing if the network is slow/unresponsive. The pro is that I can keep the existing interface. The con is that, as per current implementation, it can potentially hang Sketch, and the implementation is less uniform.makeImageDataFromUrl
return a Promise and make the whole chain of functions up torender
async as well. This allows us to usefetch
, makes the whole thing faster in cases where there are multiple images in a layout, and in general less prone to hang, but it's a breaking change.@mathieudutour Thoughts?