feat: improve TS SDK runtime performances #7096
Conversation
TomChv
force-pushed
the
feat/improve-ts-performances
branch
from
e35d7c0
to
fe36957
Compare
TomChv
force-pushed
the
feat/improve-ts-performances
branch
2 times, most recently
from
77828af
to
1988c2b
Compare
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
@TomChv how's this one going? Anything you need a hand with? |
I'm trying to figure out how to reduce the installation time of the global package, it seems using |
TomChv
force-pushed
the
feat/improve-ts-performances
branch
from
1988c2b
to
d1e14c4
Compare
|
It seems theses changes in the setup introduce issues in the tests, sometime the dependencies cannot be installed properly and it's a weird typescript error. |
|
I can reproduce the error using I'm going to try to reproduce it locally, it seems something is going wrong when using Update 1: Update 2: Update 3: Update 4: Update 5: I'm still trying to understand what is happening. I think dagger is not correctly instead in the Update 5 My changes also extends the time of Update 6 |
sdk/typescript/runtime/main.go
Outdated
| executor = "bun" | ||
| } | ||
|
|
||
| ctr = ctr.WithExec([]string{executor, "/opt/module/bin/__tsconfig.updator.ts"}) |
There was a problem hiding this comment.
Why not put this logic in Go since it's faster?
There was a problem hiding this comment.
That's a good point, however I think I should take care of that in a follow up
You may have forgotten but putting the whole |
|
I would very much like to see a version of this land in the next milestone, Even if we don't finish everything, any improvement that we can make will go a long way. |
|
I tried this with a simple TypeScript pipeline for a Next.js app: /**
* Build automation powered by Dagger
*/
import { dag, Directory, Secret, object, func } from "@dagger.io/dagger"
@object()
class App {
/**
* Publish image: dagger call image --source=.:app --token=env:GH_TOKEN
*/
@func()
async image(source: Directory, token: Secret): Promise<string> {
return dag
.container()
// https://hub.docker.com/_/node/tags?page=&page_size=&ordering=&name=lts
.from("node:lts-iron")
// https://github.com/krallin/tini
.withExec(["apt", "update"])
.withExec(["apt", "install", "--yes", "tini"])
.withDirectory("/app", source)
.withWorkdir("/app")
.withExec(["npm", "install"])
.withExec(["npm", "run", "build"])
.withEntrypoint(["tini", "--", "npm", "run", "start"])
.withRegistryAuth("ghcr.io", "gerhard", token)
.publish("ghcr.io/gerhard/nextjs")
}
}I am connecting to a remote Docker instance via SSH: Client: Docker Engine - Community
Version: 26.0.0
OS/Arch: darwin/amd64
Server: Docker Engine - Community
Engine:
Version: 26.1.3
OS/Arch: linux/amd64
containerd:
Version: 1.6.31
runc:
Version: 1.1.12Linux 6.8 x86_64 host where Docker / Dagger is running has:
These are the results for various runs:
Based on the above measurements, I observed one improvement, 4th run, when I just kept updating a comment in the module, which was Before this change, that was inconsistent: 5th run was only I also observed one deterioration: 1st run - no cache was Here are Dagger Cloud traces for the interesting runs above:
Based on the above measurements, this does not seem a net performance improvement to me. What do you think @TomChv:
|
|
Thanks for your amazing feedbacks! It really helps me understand what's going on with a real world use case.
Yep absolutely, I could hit the same kind of behaviour and flakiness on my tests! Specially between 4 and 5. The improvement of 40% to 10% is also something I got which confirm my work! I could hit a 2x improvement with an old version but it seams that it wasn't supporting every cases (to be more specfic: the
Yes! Perfect, I needed the run and the traces, to see how much time take every runs and which steps is longer.
I think what I've done in this PR is a good start for further improvement, there's still some mystery like this example below where I cannot understand how is that possible. 
I'm super confused about the fact that yarn was waaay slower than npm when the I think I'm missing something about yarn, which keep us from using the cache. I'm thinking that at some point, we could hit higher performance if we were running the TS SDK in runtime language but that seams quite complex to do. I think it's still worth merging the PR, because it makes the module much clearer but there's still a long way to go before getting a super fast TS SDK |
|
I'm also having a concern about the CI and the flakiness this PR introduce, on multiple runs, I could see different tests passing & failing, with always this kind of error: Meaning that there's an issue with yarn cache and I cannot understand why is that so, this is something that isn't happening with npm at all. As a fix, I'm trying to use yarn v4 in order to fixe both cache and download performance, according to some benchmark I was it can make things much faster I also disabled yarn cache file to avoid flackiness, it might still become faster than npm thanks to yarn v4 hopefully the CI will be green |
|
By doing so, I could successfully get a green success on initialization, which is a good news 
|
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Move SDK depenencies installation before copying source code so we do not re downaload dependencies after a user changes (make things much faster because we benefit from cache). Update runtime to only use dagger command. I did some tests, from a fresh engine we got 2 to 3 time differences. Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
TomChv
force-pushed
the
feat/improve-ts-performances
branch
from
af52ea4
to
2013233
Compare
|
@gerhard It seems I could fix the CI, could you do one more benchmark with your example just to see if there's any changes? I think it might get slower, I'll need to find why the yarn cache fails with collision. |
|
Conclusion of this PR This PR changed several internal things in the TypeScript SDK runtime. I'm not sure I could succeed in my path to reduce time by 2 but I still want to give a summary of what will happens with this PR:
Few things to do as next steps
|
| ). | ||
| WithWorkdir(filepath.Join(ModSourceDirPath, subPath)) | ||
|
|
||
| return t.setupModule(ctx, base, detectedRuntime, name) |
There was a problem hiding this comment.
Good refactor here 🎉
This was very hard to understand before, super glad to see this cleaned up! ❤️
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Thanks for the approval, I'm gonna merge the PR once CI is green. |
|
A lot of helpful detail in your recent comments @TomChv, thank you for going the extra mile 🙌 I am doing one more test run now:
👆 Links to Dagger Cloud Traces inline. 💪 LGTM 🚀 |
Thanks, I'll still merge the PR no matter what, the improvement in the readability is super important and will help for further optimizations. |
|
Before merging, can you please add a |
💪 LGTM 🚀 Thanks for these amazing benchmark! I'll add the changie and we can merge it |
Signed-off-by: Tom Chauveau <tom@epitech.eu>
|
Release note added in c423bf9 |
|
This introduced a bug in provisioning, we will not be able to release without a fix @TomChv: https://github.com/dagger/dagger/actions/runs/9193090706/job/25283718931#step:6:11 
|
dagger#7096 introduced a regression by using `yarn install --production`. If the field `packagaManager` is set, it might lead to failure because yarn post v1.22.11 doesn't support the `--production` flag anymore, it has been removed in favor of `yarn workspaces focus --production`. However, depending on the yarn major version (v2, v3, v4), the options are different but also the results and the way they handle subdependencies. It seems `npm` is the only package manager that actually force install all the sub dependencies, which ensure the TS SDK runtime properly works. Signed-off-by: Tom Chauveau <tom@epitech.eu>
* fix(ts-sdk) rollback to npm to ignore packageManager field #7096 introduced a regression by using `yarn install --production`. If the field `packagaManager` is set, it might lead to failure because yarn post v1.22.11 doesn't support the `--production` flag anymore, it has been removed in favor of `yarn workspaces focus --production`. However, depending on the yarn major version (v2, v3, v4), the options are different but also the results and the way they handle subdependencies. It seems `npm` is the only package manager that actually force install all the sub dependencies, which ensure the TS SDK runtime properly works. Signed-off-by: Tom Chauveau <tom@epitech.eu> * chore: add release note Signed-off-by: Tom Chauveau <tom@epitech.eu> * feat: move corepack init inside SDK installation Signed-off-by: Tom Chauveau <tom@epitech.eu> * fix: remove `--production` on user's install deps Signed-off-by: Tom Chauveau <tom@epitech.eu> --------- Signed-off-by: Tom Chauveau <tom@epitech.eu> Co-authored-by: Tom Chauveau <tom@epitech.eu>
Following #7093, I found that most of the time is spent downloading dependencies, it seems I can drastically reduce the download time using
yarnand the--productionflag to only install production dependencies.This is a first step toward improving TS performances, we might switch to
pnpmlater, this require some tests though because I'm hitting an issue (see #7093 (comment))