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

[Feature/Question] Reuse JsRuntime and MainWorker, or create a new v8::Isolate after running execute_script #17861

Closed
seanwatters opened this issue Feb 21, 2023 · 9 comments

Comments

@seanwatters
Copy link
Contributor

I've been working on a project where I would like to be able to embed Deno in a web server written in Rust, so that I can post a string of JavaScript code and have it executed in the Deno/v8 sandbox.

The challenge I'm facing right now is that I can't seem to find a good way to reuse a JsRuntime or MainWorker instance for each request without creating a memory leak (I believe the v8::Isolate is filling up and getting sad when I run execute_script on the same JsRuntime/MainWorker instance repeatedly).

// shared `JsRuntime` example that leads to memory leak when `run_js` is called in `actix_web::Responder`s

thread_local! {
    static JS_RUNTIME: RefCell<JsRuntime> = {
        RefCell::new(JsRuntime::new(RuntimeOptions::default()))
    }
}

fn run_js(src: &str) -> DetachedBuffer {
    JS_RUNTIME.with(|js_runtime| {
        let mut js_runtime = js_runtime.borrow_mut();        

        let result = js_runtime.execute_script("<usage>", src).unwrap();

        let scope = &mut js_runtime.handle_scope();
        let local = v8::Local::new(scope, result);

        serde_v8::from_v8::<DetachedBuffer>(scope, local).unwrap()
    })
}

I've done some tests and have a repository here to demonstrate the situation.

The results were:

Pattern Avg. Latency
v8 (new Isolate created for every request) 4.23ms
JsRuntime (created on every request) 7.7ms
MainWorker (created on every request) 17.89ms
JsRuntime (shared) 0.27ms
MainWorker (shared) 0.23ms

Based on that information, the slowest is initializing MainWorker on every request (which makes sense), JsRuntime a bit better, and v8 with a new Isolate is the fastest of the "re-initialize stuff on every request." But, when the JsRuntime and MainWorker can be reused for multiple requests we get sub-millisecond latency. Based on that, I would love to have an option where a v8::Isolate could be reused and maybe cleared manually? or at the end of .execute_script()?

I'm very new to deno_core and deno_runtime so there may already be a way for me to do what I want to do without creating a memory leak, but I'm also not a v8 expert, so I don't know if there's even such a thing as clearing the current v8::Isolate or if creating a new one on each request is always necessary.

Thanks!

@seanwatters seanwatters changed the title [Feature/Question] Create a new v8::Isolate without initializing a new JsRuntime or MainWorker [Feature/Question] Reuse JsRuntime or MainWorker or create a new v8::Isolate after running execute_script Feb 21, 2023
@seanwatters seanwatters changed the title [Feature/Question] Reuse JsRuntime or MainWorker or create a new v8::Isolate after running execute_script [Feature/Question] Reuse JsRuntime and MainWorker, or create a new v8::Isolate after running execute_script Feb 21, 2023
@bartlomieju
Copy link
Member

bartlomieju commented Feb 21, 2023

I'm not sure I fully understand the request, but... Every instance of MainWorker or JsRuntime is tied to specific instance of v8::Isolate. The slowdown you see when using different APIs is because of the fact that v8::Isolate is very bare-bones - if you wanted to execute ES modules, you are on your own, you'd have to implement it yourself. JsRuntime has an implementation of ES module system, while MainWorker ships with the whole Deno runtime inside of it. (There are more differences, but you can think of v8 crate as the lowest level, deno_core as a foundation for any real-world integration, while deno_runtime is full-fledged runtime with various APIs available in it).

There's no way to discard v8::Isolate from JsRuntime and replace it with a new one.

@seanwatters
Copy link
Contributor Author

seanwatters commented Feb 21, 2023

@bartlomieju, apologies for initial description not being super clear.

Given the performance difference between the v8 with an Isolate reset, and the reuse of a single Isolate (in the shared JsRuntime example), ideally I would be able to reuse a single v8::Isolate for multiple requests.

Maybe the better question to ask would've been: is there a way to use the pattern in the above code snippet, without creating a memory leak? And am hoping to better understand why calling repeatedly .execute_script() on a single JsRuntime instance creates a memory leak in my example.

(Just for additional context, I'm including the "purely v8" and full shared JsRuntime examples below)

// v8
use actix_web::{get, App, HttpServer, Responder};

use deno_core::DetachedBuffer;
use deno_core::serde_v8;
use deno_core::v8;

fn run_js(src: &str) -> DetachedBuffer {
    let isolate = &mut v8::Isolate::new(v8::CreateParams::default());
    
    let handle_scope = &mut v8::HandleScope::new(isolate);
    
    let context = v8::Context::new(handle_scope);
    
    let scope = &mut v8::ContextScope::new(handle_scope, context);

    let code = v8::String::new(scope, src).unwrap();

    let script = v8::Script::compile(scope, code, None).unwrap();
    let result = script.run(scope).unwrap();

    serde_v8::from_v8::<DetachedBuffer>(scope, result).unwrap()
}

#[get("/js")]
async fn greet() -> impl Responder {
    format!("Result: {:?}", run_js("new Uint8Array([0,1,2,3])").as_ref())
}

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    let platform = v8::new_default_platform(0, false).make_shared();

    v8::V8::initialize_platform(platform);
    v8::V8::initialize();

    HttpServer::new(|| {
        App::new().service(greet)
    })
    .bind(("127.0.0.1", 8080))?
    .run()
    .await
}
// shared JsRuntime
use actix_web::{get, App, HttpServer, Responder};

use std::cell::RefCell;

use deno_core::DetachedBuffer;
use deno_core::JsRuntime;
use deno_core::RuntimeOptions;
use deno_core::serde_v8;
use deno_core::v8;

thread_local! {
    static JS_RUNTIME: RefCell<JsRuntime> = {
        RefCell::new(JsRuntime::new(RuntimeOptions::default()))
    }
}

fn run_js(src: &str) -> DetachedBuffer {
    JS_RUNTIME.with(|js_runtime| {
        let mut js_runtime = js_runtime.borrow_mut();        

        let result = js_runtime.execute_script("<usage>", src).unwrap();

        let scope = &mut js_runtime.handle_scope();
        let local = v8::Local::new(scope, result);

        serde_v8::from_v8::<DetachedBuffer>(scope, local).unwrap()
    })
}

#[get("/js")]
async fn greet() -> impl Responder {
    format!("Result: {:?}", run_js("new Uint8Array([0,1,2,3])").as_ref())
}

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    HttpServer::new(|| {
        App::new().service(greet)
    })
    .bind(("127.0.0.1", 8080))?
    .run()
    .await
}

@bartlomieju
Copy link
Member

Maybe the better question to ask would've been: is there a way to use the pattern in the above code snippet, without creating a memory leak?

I don't think that's a memory leak - when you are executing this code, V8 treats every execution of the provided script as separate and thus each execution eats up more memory. Once you evaluate the code, V8 stores its source which IIRC will not be freed until the context it belongs to is destroyed. JsRuntime has a single "global context" that lives as long as the runtime itself, so V8 keeps holding into that source code forever.

Currently there's no way to destroy a context from JsRuntime and create a new once, as a lot of infrastructure depends on the assumption that there's a "global context". @andreubotella is working on ShadowRealm and added support for deno_core::JsRealm, which might be more appropriate for your use case.

@seanwatters
Copy link
Contributor Author

Thank you so much for pointing me towards JsRealm! It looks like this is working without giving me the OOM error:

fn run_js(src: &str) -> DetachedBuffer {
    JS_RUNTIME.with(|js_runtime| {
        let mut js_runtime = js_runtime.borrow_mut();

        let isolate = js_runtime.v8_isolate();
        let js_realm: JsRealm;
        
        {
            let mut handle_scope = v8::HandleScope::new(isolate);
    
            let context = v8::Context::new(&mut handle_scope);
            let mut context_scope = v8::ContextScope::new(&mut handle_scope, context);
    
            let global = v8::Global::new(&mut context_scope, context);
            js_realm = JsRealm::new(global);    
        };

        let result = js_realm.execute_script(isolate, "<usage>", src).unwrap();

        let scope = &mut js_realm.handle_scope(isolate);
        let local = v8::Local::new(scope, result);

        serde_v8::from_v8::<DetachedBuffer>(scope, local).unwrap()
    })
}

I think you're right that it's not actually a leak per se, and in fact the large amount of source code being stored is the cause for the OOM error. For reference (in case someone else comes across the issue) this is the error I was getting when running without a JsRealm:

    Finished dev [unoptimized + debuginfo] target(s) in 3.41s
     Running `target/debug/js_runtime_shared`

<--- Last few GCs --->

[46642:0x130018000]     7020 ms: Scavenge 359.0 (395.2) -> 350.5 (398.9) MB, 7.7 / 0.0 ms  (average mu = 0.962, current mu = 0.967) allocation failure; 
[46642:0x130018000]     7246 ms: Scavenge 366.2 (402.4) -> 357.8 (406.7) MB, 6.3 / 0.0 ms  (average mu = 0.962, current mu = 0.967) allocation failure; 
[46642:0x130018000]     7475 ms: Scavenge 373.5 (409.7) -> 365.0 (413.9) MB, 5.1 / 0.0 ms  (average mu = 0.962, current mu = 0.967) allocation failure; 


<--- JS stacktrace --->


#
# Fatal javascript OOM in MarkCompactCollector: young object promotion failed
#

[1]    46641 trace trap  npm run js_runtime_shared

Closing issue out!

@bartlomieju
Copy link
Member

Kudos to @andreubotella for implementing JsRealm

@andreubotella
Copy link
Contributor

andreubotella commented Feb 22, 2023

Note that both JsRuntime::execute_script and JsRealm::execute_script basically make a v8::String from the passed &str and then compile it into a v8::Script and run it. This v8::String doesn't seem to be cached. So you could save on those string copies by caching the v8::Script (if you're using a single realm/context) or the v8::String (which also works across realms).

@seanwatters
Copy link
Contributor Author

seanwatters commented Feb 22, 2023

@andreubotella, great to know!

One quick thing that I did notice: it looks like the documentation / comments in the core/runtime.rs say to use JsRuntime::create_realm(), however when I use that method, I run into the same OOM error.

Is it possible that the global context (which I believe is the difference maker) is still getting linked to the created JsRealm in a way that is causing the source code to be saved from even the JsRealm's execute_script?

// works great but doesn't come with `init_cb` or `init_extension_js`
fn run_js(src: &str) -> DetachedBuffer {
    JS_RUNTIME.with(|js_runtime| {
        let mut js_runtime = js_runtime.borrow_mut();

        let isolate = js_runtime.v8_isolate();
        let js_realm: JsRealm;
        
        {
            let mut handle_scope = v8::HandleScope::new(isolate);
    
            let context = v8::Context::new(&mut handle_scope);
            let mut context_scope = v8::ContextScope::new(&mut handle_scope, context);
    
            let global = v8::Global::new(&mut context_scope, context);
            js_realm = JsRealm::new(global);    
        };

        let result = js_realm.execute_script(isolate, "<usage>", src).unwrap();

        let scope = &mut js_realm.handle_scope(isolate);
        let local = v8::Local::new(scope, result);

        serde_v8::from_v8::<DetachedBuffer>(scope, local).unwrap()
    })
}

// has more Deno functionality, but OOM when called too many times
fn run_js(src: &str) -> DetachedBuffer {
    JS_RUNTIME.with(|js_runtime| {
        let mut js_runtime = js_runtime.borrow_mut();

        let js_realm = js_runtime.create_realm().unwrap();
        let isolate = js_runtime.v8_isolate();

        let result = js_realm.execute_script(isolate, "<usage>", src).unwrap();

        let scope = &mut js_realm.handle_scope(isolate);
        let local = v8::Local::new(scope, result);

        serde_v8::from_v8::<DetachedBuffer>(scope, local).unwrap()
    })
}

The other interesting thing, is that when I use the JsRuntime::create_realm(), I get some of the Deno API, but attempting to use classes TextDecoder and TextEncoder in the JavaScript code throws ReferenceError: TextDecoder is not defined and ReferenceError: TextEncoder is not defined.

@andreubotella
Copy link
Contributor

andreubotella commented Feb 22, 2023

I think you might be running into #17199.

JsRealm is still WIP and not really production-ready. Also, while isolates have separate memory heaps, so that dropping a v8::Isolate (and so also a JsRuntime or MainWorker) will free all memory associated with it, realms/contexts don't really.

The other interesting thing, is that when I use the JsRuntime::create_realm(), I get some of the Deno API, but attempting to use classes TextDecoder and TextEncoder in the JavaScript code throws ReferenceError: TextDecoder is not defined and ReferenceError: TextEncoder is not defined.

JsRuntime doesn't implement any web APIs or any API in the Deno.* namespace other than the Deno.core internals. Those come with deno_runtime/MainWorker. While it is possible to create a realm with such APIs by using snapshots, it's very much untested. I'll probably get around to implementing that in a couple months when we get #15760 landed.

@seanwatters
Copy link
Contributor Author

seanwatters commented Feb 22, 2023

JsRuntime doesn't implement any web APIs or any API in the Deno.* namespace other than the Deno.core internals.

I ran into this the other day and did switch to using MainWorker. The example I shared above stripped out the fact that I'm using main_worker.js_runtime.create_realm() to create the JsRealm (I'm bouncing back and forth between two projects, one that needs the web APIs and another that doesn't).

But I guess it does make sense that creating a realm from the main_worker.js_runtime wouldn't support the web APIs. Maybe a MainWorker::create_realm method would be a good option for creating JsRealms that we want to have all of the MainWorker APIs baked in?

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

No branches or pull requests

3 participants