Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tea consuming RAM in proportion to download size #338

Closed
mteam88 opened this issue Jan 25, 2023 · 15 comments · Fixed by #356
Closed

tea consuming RAM in proportion to download size #338

mteam88 opened this issue Jan 25, 2023 · 15 comments · Fixed by #356

Comments

@mteam88
Copy link

mteam88 commented Jan 25, 2023

When I run tea +rust-lang.org on a fresh Codespace, the command seems to begin installation, but then gets killed. This may be a problem with codespaces, but if it is, I have never seen it before.
image

$ sh <(curl tea.xyz) +rust-lang.org
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 13306  100 13306    0     0   316k      0 --:--:-- --:--:-- --:--:--  316k
llvm.org=14.0.6 downloading 31%
Killed

Regular rust installation works fine in codespaces.
It seems that llvm.org installation gets to 31% or so and then is Killed

@mxcl
Copy link
Member

mxcl commented Jan 25, 2023

hmm. I wonder if our download code uses too much RAM? We stream the bytes to a file so we shouldn't be keeping around a large buffer. But I imagine RAM is a tight constraint, and certainly OoM (out of memory) is a killable offense.

/cc @jhheider

@mteam88
Copy link
Author

mteam88 commented Jan 25, 2023

I'll be happy to see if that is a known issue with codespaces (I can also try a higher RAM codespace)

@mteam88
Copy link
Author

mteam88 commented Jan 26, 2023

On a higher ram (4 GB to 8 GB) code space, the installation was successful. Thanks!

@mteam88 mteam88 closed this as completed Jan 26, 2023
@mxcl mxcl reopened this Jan 26, 2023
@mxcl
Copy link
Member

mxcl commented Jan 26, 2023

Reopening because seemingly we are consuming RAM in proportion to download size. Potentially this is not working as we expect.

@jhheider
Copy link
Contributor

I would say this doesn't have the smell of a streamable interface.

The newer SubtleCrypto-based API looks like it'll take iterable BufferSources, but some algorithms require being able to inspect the whole object at once (though I suspect SHA256 isn't one of those). Still, it might stream data in, but I wouldn't be surprised if it keeps it, and, if the algorithm requires it, it must.

@jhheider
Copy link
Contributor

this suggests sha256 is streamable.

@lino-levan
Copy link
Contributor

Sha256 is streamable. It looks like std/crypto/crypto.ts with an async iterable may be able to solve this problem. I missed this when I was working on bumping the std dependency, but it doesn't buffer the whole source into memory. It instead uses a wasm implementation of sha256. If we switch over to this, we should also get a nice perf improvement from a pure javascript implementation.

if ((data as AsyncIterable<BufferSource>)[Symbol.asyncIterator]) {
  const wasmCrypto = instantiateWasm();
  const context = new wasmCrypto.DigestContext(name);
  for await (const chunk of data as AsyncIterable<BufferSource>) {
    const chunkBytes = bufferSourceBytes(chunk);
    if (!chunkBytes) {
      throw new TypeError("data contained chunk of the wrong type");
    }
    context.update(chunkBytes);
  }
  return context.digestAndDrop(length).buffer;
}

@lino-levan
Copy link
Contributor

It would probably require a refactor of the code but I think that makes sense in the short term.

In the long term, if w3c/webcrypto#73 gets addressed, we may get a native solution to this. By the time that gets merged and implemented though I wouldn't be surprised if tea moved to rust.

@lino-levan
Copy link
Contributor

This just bit me today while trying to use clang inside a vm with 4 gigs of memory. This is very painful. I will open a PR soon with the refactor of useDownload.

@mteam88
Copy link
Author

mteam88 commented Jan 30, 2023

Perhaps an off topic question, but why would tea move to rust?

@mteam88 mteam88 changed the title tea +rust-lang.org "Killed" in Github Codespaces tea consuming RAM in proportion to download size Jan 30, 2023
@mxcl mxcl closed this as completed in #356 Feb 5, 2023
@mxcl
Copy link
Member

mxcl commented Feb 5, 2023

Perhaps an off topic question, but why would tea move to rust?

@mteam88 the two reasons are binary size and performance. The only performance issue we are likely to not be able to solve with TypeScript/deno is startup time. If that can be solved then likely we won’t have any real concerns. We could always write some pieces in rust and bind them in if there are any specific sections of code that would benefit.

The binary size is less likely to be solved (I think, correct me if wrong). Currently we are ~90MB uncompressed. It's true that nowadays this is less an issue in general. But:

  1. It's a factor people will hold against us, they'll compare us to rust/go tools that are a few MB at most.
  2. I want our installer to be as fast as possible to communicate that tea aims to be as fast as possible from the get-go
  3. We aim to be in CI/CD all over the place so being smaller will win us more favor there as well as mean we are being a good citizen in general

Provided deno can mitigate these then we probably wouldn't switch. I love TypeScript and deno both.

@mteam88
Copy link
Author

mteam88 commented Feb 5, 2023

@mxcl thanks. That makes a ton of sense

@jhheider
Copy link
Contributor

jhheider commented Feb 5, 2023

There's also the fact that writing everything in rust takes 10 times as long, but it basically can't crash if it compiles[citation needed]. It's why rust programmers have such high language satisfaction. That and masochism.

@lino-levan
Copy link
Contributor

but it basically can't crash if it compiles

😅 I think this is the concept, but unfortunately due to the aforementioned increase in dev time it is quite common to take shortcuts to get things done faster (in an unsafe manner). Unfortunately see this a lot in OSS.

I am a big fan of the current plan of (nailing down the api with deno) -> (rewrite in rust) though. Seems to capture the best of both worlds.

@jhheider
Copy link
Contributor

jhheider commented Feb 5, 2023

Oh, sure. You can choose to balance dev time vs. safety. Similar to Swift in that fashion. If you sprinkle around a lot of unwrap()/!, you'll get done faster, but you've made a panic possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants