Make Runtime::new safe

[nox]

CC @tschneidereit @Ms2ger @jdm

---

This change is

[tschneidereit]

Looks good with comments addressed. Using a lazy static is a nice solution.

Previously, nox (Anthony Ramine) wrote…

Make Runtime::new safe

CC @tschneidereit @Ms2ger @jdm

---

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

src/rust.rs, line 122 [r1] (raw file):

    pub unsafe fn new(parent_rt: *mut JSRuntime) -> Runtime {
        let js_runtime = JS_NewRuntime(default_heapsize, ChunkSize as u32, parent_rt);
        assert!(!js_runtime.is_null());

We still need this assert or some other form of null check. Creating a child runtime is just as fallible as creating the parent runtime, after all.

---

src/rust.rs, line 125 [r1] (raw file):

                        assert!(JS_Init());
                        let runtime = JS_NewRuntime(
                            default_heapsize, ChunkSize as u32, ptr::null_mut());

This should use as small a heap size as possible. The only JS ever running in here should be the self-hosted code. Not sure how best to figure out a good value, though.

---

src/rust.rs, line 128 [r1] (raw file):

                        assert!(!runtime.is_null());
                        let context = JS_NewContext(
                            runtime, default_stacksize as size_t);

Much more important than the heap size, and should be much smaller. 1/16th (512kb) might well be enough, or even 1/32th (256kb).

---

src/rust.rs, line 130 [r1] (raw file):

                            runtime, default_stacksize as size_t);
                        assert!(!context.is_null());
                        TopRuntime(runtime)

It might make sense to trigger a compacting GC at this point, as no new code execution should happen in this runtime afterwards. I also wonder if we can write-protect the runtime's heap afterwards?

---

Comments from Reviewable

[tschneidereit]

@jandem informs me that both the stack and the heap are allocated lazily, so I guess my comments on their sizes can be safely ignored.

[nox]

Put back the assertion. LGTM?

Previously, tschneidereit (Till Schneidereit) wrote…

@jandem informs me that both the stack and the heap are allocated lazily, so I guess my comments on their sizes can be safely ignored.

---

Review status: 8 of 9 files reviewed at latest revision, 4 unresolved discussions.

---

src/rust.rs, line 122 [r1] (raw file):

Previously, tschneidereit (Till Schneidereit) wrote…

We still need this assert or some other form of null check. Creating a child runtime is just as fallible as creating the parent runtime, after all.

Done. That was mistakenly removed.

---

src/rust.rs, line 125 [r1] (raw file):

Previously, tschneidereit (Till Schneidereit) wrote…

This should use as small a heap size as possible. The only JS ever running in here should be the self-hosted code. Not sure how best to figure out a good value, though.

So I don't need to do anything right? Following your last comment.

---

src/rust.rs, line 128 [r1] (raw file):

Previously, tschneidereit (Till Schneidereit) wrote…

Much more important than the heap size, and should be much smaller. 1/16th (512kb) might well be enough, or even 1/32th (256kb).

Same question.

---

src/rust.rs, line 130 [r1] (raw file):

Previously, tschneidereit (Till Schneidereit) wrote…

It might make sense to trigger a compacting GC at this point, as no new code execution should happen in this runtime afterwards. I also wonder if we can write-protect the runtime's heap afterwards?

Same question.

---

Comments from Reviewable

[tschneidereit]

Yes, LGTM.

Previously, nox (Anthony Ramine) wrote…

Put back the assertion. LGTM?

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[tschneidereit]

@bors-servo r+

[bors-servo]

🔑 Insufficient privileges

[nox]

@bors-servo r=tschneidereit

[bors-servo]

📌 Commit a301cff has been approved by tschneidereit

[bors-servo]

⌛ Testing commit a301cff with merge c6f6817...

[bors-servo]

☀️ Test successful - travis