Pre-build islands - Githubissues

kitsonk commented 1 year ago

I have a couple decently sized islands (https://github.com/ts-why/tswhy/tree/main/islands) where dynamically building them on Deno Deploy takes quite a lot of time, to the point they can regularly exceed the time limit on Deploy, meaning that to gain full functionality an initial user has to reload several times.

While having "no build step" is great in some cases, it would actually be a huge performance benefit and better user experience to statically build these islands.

Also, there doesn't seem to be any easy tooling to manage the size/complexity of islands. While esbuild underneath does a great job tree shaking, the whole island build process is opaque and you can't really tell if you are dragging in a load of junk into your island which then esbuild then is shaking out (or even worse, unintended unshaken code).

lucacasonato commented 1 year ago

Yes - I agree. I want to improve this. I'd like to add an optional "pre-build" mode that emits the island code to disk.

lino-levan commented 1 year ago

Just reffing #1037 here.

jiawei397 commented 1 year ago

Very good proposal. I am also looking forward to this function. It seems that all I can do now is to improve the running CPU of my server, which is not a good idea.

jiawei397 commented 1 year ago

I now use K8S to run the docker service in CICD. After each time I go online, it usually takes more than 30 seconds for my page.

At first, I thought it was a CPU problem, because the compilation of local TS or TSX is in seconds. But after careful observation, I found that it was only after the service was started that I began to download the bin file of esbuild, which is the culprit of the slow compilation. This also explains my doubt that the performance of esbuild should not be so bad.

My current practice is to download it by running the command in advance when building the image.

deno run -A https://deno.land/x/esbuild@xx/mod.js

Now the speed of compiling TSX for the first time on the page is about the same as my local one, which is acceptable.

In the deployment of Deno Deploy, I have no voice and will not participate. It would be better if you could pre-build the file.

lino-levan commented 1 year ago

This really should not be that difficult. I'm thinking that (similar to the upgrade script) we have a build script. Before caching a module for the first time, check if it's already been built (in a _frsh folder or something) and instead of rebuilding it just read it from disk. Will have a demo by this weekend.

lino-levan commented 1 year ago

With some small modifications to ./src/server/bundle.ts and ./src/server/context.ts. We can get away with a build script as complicated as this:

import { ServerContext } from "$fresh/src/server/mod.ts";

import twindPlugin from "$fresh/plugins/twind.ts";

import manifest from "./fresh.gen.ts";
import twindConfig from "./twind.config.ts";

const ctx = await ServerContext.fromManifest(manifest, { plugins: [twindPlugin(twindConfig)] });

const bundle = await ctx.bundler.cache()

try {
  await Deno.remove("./_frsh", {recursive: true})
}
catch(err) {
  if(!(err instanceof Deno.errors.NotFound)) {
    throw err
  }
}

await Deno.mkdir("./_frsh", {
  recursive: true
})

for(const [filename, contents] of bundle) {
  await Deno.writeFile("./_frsh"+filename, contents);
}

This code is really gross (and so are the modifications you need to make for this to work) but it does in fact work. I'm going to try to clean it up a bit and open a PR soon.

lino-levan commented 1 year ago

There are a few issues with the current setup:

This script hangs after it's done generating the files. Probably has something to do with esbuild still hanging around in memory somewhere. This should quite literally be one or two lines of code.
This should abstracted away a la dev.tswhich is not something that is currently happening.
The user needs to define plugins both in the build script and in the main.ts script which is unfortunate. Either we do some """clever""" string parsing and dynamic imports to avoid this or we just accept that this is the case since the vast majority of people will not want a build step.

Thoughts @lucacasonato @kitsonk?

lino-levan commented 1 year ago

Just one more thought: This code builds everything at the moment.

Should we only be pre-building islands? This would change (and simplify) a lot of the requirements here.

ayame113 commented 1 year ago

What do you think about caching build results to Deno KV?

Personally, I'm not a big fan of committing build results to GitHub or setting up a build process on CI. Caching the build output in Deno KV and updating the cache every time a new deployment is created is good enough for me to improve cold start.

iuioiua commented 1 year ago

Interesting idea... Ok, let's try both - one PR targeting the traditional method (Lino's) and another PR using Deno KV. May the best method win.

lino-levan commented 1 year ago

What do you think about caching build results to Deno KV?

Definitely interesting (and I think we should do both!), but I think a normal build step is kind of needed for deploying outside of Deno Deploy.

iuioiua commented 1 year ago

Definitely interesting (and I think we should do both!), but I think a normal build step is kind of needed for deploying outside of Deno Deploy.

Why's that? Deno KV would work inside or outside of Deno Deploy.

lino-levan commented 1 year ago

Curious to see the performance of the KV-based solution is what I mean (considering islands can get pretty big).

ayame113 commented 1 year ago

I agree to allow users to choose between caching to static file and caching to KV. (KV seems to have a 64KiB limit.)

For example like this?

import { start } from "$fresh/server.ts";
import twindPlugin from "$fresh/plugins/twind.ts";

import manifest from "./fresh.gen.ts";
import twindConfig from "./twind.config.ts";

await start(manifest, {
  plugins: [twindPlugin(twindConfig)],
  useKvCache: false, // Cache build results to Deno KV
  usePreBuild: true, // Output build results to static files
});

iuioiua commented 1 year ago

Oh! I forgot about that KV limit. That might be a dealbreaker... Either way, we should only stick to one means of caching.

kitsonk commented 1 year ago

Deno KV has a per item 2KiB key limit and 64KiB value limit. Currently Deploy limits storage to 2GiB per deployment.

Deno KV really isn't suitable for largish per deployment version static content. It is suitable for data that changes independent of a deployment instance and is dynamic. It is far more suitable for Fresh to be somewhat abstracted from it, and to allow pre-building and allow a user to determine how best to host it, something that is overridable via an environment variable defaulting to GitHub, that way be it S3 or some other static storage that makes sense for the user, then it is up to them.

If Deploy ever provides static asset storage akin to S3 then it might make sense to tightly couple it into Fresh.

ayame113 commented 1 year ago

I certainly agree that static asset storage like S3 is a better choice. It seems that the file size of the build result can exceed the size limit of the KV value.

However, deno deploy doesn't provide static file storage at the moment, so for now I'd like to use KV to store the build output. The build output file size issue can be circumvented by using kv-toolbox, a library written by @kitsonk . Allowing the user to freely choose between local files, KV, S3, etc. is enough for me.

In summary, I'm starting to think it might be nice to make the part where build results are saved and loaded configurable by the user.

Edit: If an edge cache layer is planned to be added, I think it might be a good idea to use that too. https://github.com/denoland/deploy_feedback/issues/74

ayame113 commented 1 year ago

Note: My rough measurements are that it takes 1.5s to load the wasm file and 3.5s to run esbuild.

kitsonk commented 1 year ago

The build output file size issue can be circumvented by using kv-toolbox, a library written by @kitsonk .

You do realise the I, the author of the tool is also suggesting that using Deno KV as a static cache isn't such a good idea. Just because you can, doesn't mean you should.

lucacasonato commented 1 year ago

Please hold off on this. Things are in the works to make KV more suitable for this in the not too distant future :)

ayame113 commented 1 year ago

Things are in the works to make KV more suitable for this in the not too distant future :)

Glad to hear that! I will wait for it.

tlgimenes commented 1 year ago

Adding my two cents in here.

I had the same issue and went for the "write to disk" approach in my forked version of fresh. After one week of using it I'd like to say it's not ideal. Rebases in github are a nightmare and I didn't really want to build a CI for deploying my code.

For me, the ideal solution for this would be using Deno's Cache API, like I've described in here: https://github.com/denoland/deploy_feedback/issues/74#issuecomment-1557658201. This means we have no build step, and a good caching for these assets.

In the meanwhile I'm thinking on changing the approach. I'm rolling back my fork and making the following:

esbuild outputs a file named . This file contains all assets generated during the build process. I'm going to add this to my fork so when I call /_frsh/js/{deploy_id}/metafile.json, I get the metafile.
I'll add chron that asks for this file every minute or so. It fetches the metafile.json and fetches all assets, storing them somewhere.
I'll add a cloudflare in front of Deno Deploy so I can serve these assets without hitting my Deno Deploy and risking spinning up a new isolate.

A nice side effect of this approach is that now I'll have the metafile.json and I'll be able to use tools like esbuild bundle analyser. Maybe this change could be usefull for denoland/fresh too.

lino-levan commented 1 year ago

One pretty large negative to doing build caching in KV is that the visibility issue is not solved. It's pretty hard to answer "How large are my islands?" if they can't be statically built.

ayame113 commented 1 year ago

What do you think about caching build results to Deno KV?

Thanks to everyone who gave suggestions here. KV doesn't seem very good way to cache build output.

However, I was curious about how this would work, so I tried implementing a KV-based approach myself as userland middleware. https://gist.github.com/ayame113/0c1fa4ad9b077c5cf1f5ad6a8654a38b

It's working well for me so far, so I would like to use this as a temporary workaround until optimal cache storage is implemented in the future. (The island size is displayed on the dashboard - It's 48.2 KiB.)

Thanks again to everyone who gave their opinion here!

tlgimenes commented 1 year ago

Hi @ayame113 , thanks for the gist! I used a similar idea on my forked fresh version. Also, I added the esbuild's metafile to address @lino-levan issue on islands visibility. The metafile with esbuild's bundle analyser is a very helpfull tool.

More info on the forked version in here: https://github.com/deco-cx/fresh/pull/7

zaynetro commented 1 year ago

Here is my take on prebuilding: https://github.com/denoland/fresh/pull/1450

The changes simply allow the users to generate a snapshot and persist it somehow.

denoland / fresh

Pre-build islands #1062