Skip to content
This repository has been archived by the owner on Jun 29, 2023. It is now read-only.

feat(registry): Add on the fly compilation #1902

Closed
wants to merge 5 commits into from

Conversation

AnInternetTroll
Copy link
Contributor

@kt3k
Copy link
Member

kt3k commented Oct 6, 2021

I'll do some performance check for this change on Deploy. deno.land/x/swc has 5+MB wasm dependency and that would impact the performance of the start up time of the worker

worker/registry.ts Outdated Show resolved Hide resolved
@kt3k
Copy link
Member

kt3k commented Oct 6, 2021

Regarding the design aspect, this change responds with javascript context for the path/to/foo.ts. That would work for the browsers, but might be confusing to the users.

@AnInternetTroll
Copy link
Contributor Author

Regarding the design aspect, this change responds with javascript context for the path/to/foo.ts. That would work for the browsers, but might be confusing to the users.

IMO as long as import { bold } from "https://deno.land/std@0.110.0/fmt/colors.ts" works then it doesn't matter that much to the user. If they're in an environment where TypeScript is important (like deno) then it will do all the background magic of adding types, but in theory it shouldn't change the behavior of the code.

@kt3k
Copy link
Member

kt3k commented Oct 11, 2021

This seems to slow down the start up time of the isolate from about 1.5 sec to 5.0 sec from my location (my location already has 1.0 sec penalty on start up BTW).

I'm not sure how much impact it does have to the service. Can't predict for sure whether it's safe to merge this or not (should be merged very carefully anyway)

Some numbers to share: deno.land has about 66,000 reqs/hour at the peak today. It's 18.3 reqs/sec. 3.5 sec penalty means additional 64.2 reqs are on hold during deployment (if I understand the deployment process correctly). These numbers are for the entire regions, not a single region. (Deno Deploy has 25 regions, but I guess loads wouldn't be distributed evenly)

@kt3k
Copy link
Member

kt3k commented Oct 11, 2021

Regarding the functional aspect, I was able to use some of std/collections functions in the browser. The compilation itself seems working! 👍

An example I was able to run on a browser:

<script type="module">
  import { sortBy } from "https://dry-horse-89.deno.dev/std@0.110.0/collections/mod.ts";

  const people = [ 
    { name: "Anna", age: 34 },
    { name: "Kim", age: 42 },
    { name: "John", age: 23 },
  ];  
  const sortedByAge = sortBy(people, (it) => it.age);
  console.log(sortedByAge);
</script>

(I deployed this branch at https://dry-horse-89.deno.dev)

@CLAassistant
Copy link

CLAassistant commented Mar 10, 2022

CLA assistant check
All committers have signed the CLA.

@UltiRequiem
Copy link
Contributor

I really would like this feature, is there something I can do to help?

@crowlKats
Copy link
Member

Ideally this would not be on the fly, but rather when a tag is created

@AnInternetTroll
Copy link
Contributor Author

It's a part of the roadmap on the backend. So you should look in that repo first, then we can either change this PR to use that saved stripped version or make a new PR

@josephrocca
Copy link

@crowlKats Could/should this perhaps be done lazily? So we keep this pull request, but then add another which optimises it by saving the transpiled JS code to $NAME/versions/$VERSION/stripped/$FILEPATH.json, and of course tries to serve from there before transpilation?

@ayame113
Copy link
Contributor

ayame113 commented May 18, 2022

I tried deploying this and it looks like the isolate start time has improved to 3.10ms.
Since there are no performance issues anymore, isn't it okay to merge this?

image

To store the lazily transpiled JS code, I think the cache layer suggested by denoland/deploy_feedback#74 is useful.

// JSX and TSX content type fix
if (
remoteUrl.endsWith(".jsx") &&
!resp2.headers.get("content-type")?.includes("javascript")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that resp2.headers.get("content-type") is repeated multiple times. Why don't you store it in a variable?

} else if (
remoteUrl.endsWith(".tsx") &&
!resp2.headers.get("content-type")?.includes("typescript")
headers.get("accept")?.includes("javascript") ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headers.get("accept") is also repeated.

@ry
Copy link
Member

ry commented Nov 22, 2022

closing because stale

@ry ry closed this Nov 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serve precompiled JavaScript to the browser
9 participants