Run V8 on separate thread

[bnoordhuis]

Rationale, implementation and known bugs are documented in DESIGN.md but the elevator pitch is that Ruby and V8 don't like sharing the same system stack.

mini_racer_extension.cc has been split into mini_racer_extension.c and mini_racer_v8.cc. The former deals with Ruby, the latter with JS.

This work has been sponsored by Discourse.

---

I'm sure I'll tweak some of the details before merging but it's by and large done and ready for review.

Maybe good to mention that I lifted serde.c from one of my own projects; it's designed to be #include'd in other C source files. There's a serde_test.c that I didn't include (no good way to run) but happy to add anyway.

I hope the code is self-explanatory but if not, let me know and I'll add explainers, either as code or review comments.

[bnoordhuis]

todos: work around missing pthread_barrier_t and pthread_condattr_setclock on macOS, figure out why test_date_nan fails on arm64. fixed

How is the truffleruby/graalvm thing wired up? It's failing like this:

/home/runner/work/mini_racer/mini_racer/test/mini_racer_test.rb:9:in `<class:MiniRacerTest>': undefined method `set_flags!' for MiniRacer::Platform:Class (NoMethodError)
Did you mean?  set_flag_as_str!

I guess I could add back set_flag_as_str! but if it's some kind of monkey-patching shim, then that's probably just one of many issues.

Note to self: serde should also support WeakMap/WeakSet/WeakRef, at least in the JS->Ruby direction. fixed

[SamSaffron]

Thanks so much Ben, @eregon will be across the truffle stuff I think, we probably just want to disable the features that are not supported

Will look at everything a lot more carefully once the new year kicks in.

Do we really need to give up on single threaded mini racer contexts when the flag is specified? can we somehow maintain that as well, it is handy for forking servers?

[bnoordhuis]

Do we really need to give up on single threaded mini racer contexts when the flag is specified?

Fork-then-start-V8 still works, it's start-V8-then-fork that's incompatible with threads; the V8 thread disappears and it's not safe to reinitialize V8 again. I don't think V8 lets you even if you wanted to.

I could add a mode where Ruby and V8 run on the same thread but then we're back to the problem this PR sets out to solve.

---

Personal opinion: preforking is one of those optimizations that seem great in the abstract but are only so-so in practice because of the interaction between garbage collectors and copy-on-write memory.

Forking is what I started with when I first wrote the runtime for Deno Deploy but it quickly became apparent that starting a new process is much faster.

In prefork mode, the first GC cycle sets off a massive CoW storm. It was slower (by a lot!) than just starting from scratch, even when accounting for all the JS code that needs to be loaded. Parsing a few MiBs of source code is faster than taking 40,000 page faults in a row.

[eregon]

How is the truffleruby/graalvm thing wired up? It's failing like this:

See

mini_racer/lib/mini_racer.rb

Line 5 in 8d1e66c

require "mini_racer/truffleruby"

and https://github.com/rubyjs/mini_racer/blob/main/lib/mini_racer/truffleruby.rb
So it's reimplementing the C extension with Ruby code, no monkey patching.

For set_flags!, I think it would be good to share this bit of code:

mini_racer/lib/mini_racer.rb

Lines 94 to 122 in 8d1e66c

	class Platform
	class << self
	def set_flags!(*args, **kwargs)
	flags_to_strings([args, kwargs]).each do |flag|
	# defined in the C class
	set_flag_as_str!(flag)
	end
	end
	private
	def flags_to_strings(flags)
	flags.flatten.map { |flag| flag_to_string(flag) }.flatten
	end
	# normalize flags to strings, and adds leading dashes if needed
	def flag_to_string(flag)
	if flag.is_a?(Hash)
	flag.map do |key, value|
	"#{flag_to_string(key)} #{value}"
	end
	else
	str = flag.to_s
	str = "--#{str}" unless str.start_with?('--')
	str
	end
	end
	end
	end

for example by moving it above

mini_racer/lib/mini_racer.rb

Lines 4 to 5 in 8d1e66c

	if RUBY_ENGINE == "truffleruby"
	require "mini_racer/truffleruby"

Or skip the added MiniRacer::Platform.set_flags! :stress_snapshot on truffleruby since anyway it's not relevant there.

[eregon]

FWIW on TruffleRuby there is no known issue to run JS code on the same stack as Ruby code, and in fact it's more efficient to do so.
So it would be great if running V8 on a separate thread is mostly a transparent thing, and as much as possible not a user-facing concern.

[nightpool]

In prefork mode, the first GC cycle sets off a massive CoW storm. It was slower (by a lot!) than just starting from scratch, even when accounting for all the JS code that needs to be loaded. Parsing a few MiBs of source code is faster than taking 40,000 page faults in a row

surely refork fixes this though?

[SamSaffron]

Hi Ben,

Was running this locally, this spec:

def test_pipe_leak
# in Ruby 2.7 pipes will stay open for longer
# make sure that we clean up early so pipe file
# descriptors are not kept around
context = MiniRacer::Context.new(timeout: 1000)
10_000.times { |i| context.eval("'hello'") }
end

Takes basically forever, we are getting about 4 evals per second.

What can we do to speed this up? I think we should aim to be able to do 10k evals a second maybe more... especially now that serialization is a lot more efficient.

[SamSaffron]

should we be checking return values?

[bnoordhuis]

They can't really error, they're not PTHREAD_MUTEX_ERRORCHECK locks; logic errors result in deadlock. I could call abort() if you want.

[bnoordhuis]

I've added back a single-threaded mode but it comes with a huge caveat and it's a pre-existing condition.

The Ruby scheduler before 3.4.0 clobbers thread-local variables and that trips up V8 badly. Debug builds catch it every time but release builds seemingly work okay until they don't. There's no real fix except to not use single-threaded mode.

test_pipe_leak [..] Takes basically forever, we are getting about 4 evals per second

Are you testing on macOS? I implemented the timeout argument using a watchdog thread that's spun up and down per .eval call. It's plenty fast on Linux (I can spin up 10k threads on my machine in under a second) but pthread performance is notoriously slow on macOS.

I could either:

• switch to pthread_cond_timedwait but that won't work for single-threaded mode and precludes fixing timeouts inside long-running Ruby code

• change it back to the old ruby watchdog thread approach. Ruby uses N:M threading so it's possible it's faster. On the other hand, a wayward thread that doesn't release the GVL will block the watchdog thread from running. Native threads are immune to that.

[SamSaffron]

I can confirm it is the watchdog that is making it slow, I am on linux, thread creation is ultra fast. Actually my guess is the delay here is due to speed.

My theory here is that we are signalling the timeout thread it is done prior to it actually running leading to it timing out vs terminating due to signal.

I can kind of confirm that by upping timeout out to 1_000_000 in:

  def test_pipe_leak
    context = MiniRacer::Context.new(timeout: 1_000_000)
    10_000.times { |i| context.eval("'hello'") }
  end

This appears to fix it for me:

    for (;;) {
        if (c->wd.cancel)
            break;
        pthread_cond_timedwait(&c->wd.cv, &c->wd.mtx, &deadline);
        if (c->wd.cancel)
            break;
        if (deadline_exceeded(deadline)) {
            v8_terminate_execution(c->pst);
            break;
        }
    }

double check if we are in a cancelled state prior to waiting for the first time.

[bnoordhuis]

Oh, that's a good observation. Yes, that change makes it about 3.5x faster locally. PR updated with your suggestion.

[SamSaffron]

Any idea about the musl failures

 /usr/lib/gcc/aarch64-alpine-linux-musl/14.2.0/../../../../aarch64-alpine-linux-musl/bin/ld: cannot find /usr/local/bundle/gems/libv8-node-22.7.0.4-aarch64-linux/vendor/v8/aarch64-linux-musl/libv8/obj/libv8_monolith.a: No such file or directory
collect2: error: ld returned 1 exit status
gmake: *** [Makefile:265: mini_racer_extension.so] Error 1

It feels tooling related vs any actual bug.

---

I think I am good to merge this now and kick off a --pre gem so we can confirm this resolves the segfault we were seeing.

Regarding the watchdog, a microoptimisation could be to keep it alive for longer and waiting for the next eval for up to say 10 seconds. signal it that it needs to start watching again. That optimises for lots of small operations. That said, I am not sure we need to worry about this for now.

Regarding truffle, @eregon any chance you can have a look, it looks like the polyfill is missing set_flags! We can wait I guess till after we merge.

Ben feel free to merge this tomorrow.

[eregon]

I made #326, since this PR removes the set_flags! defined in Ruby there will be no duplication after merging these 2 PRs.

So please merge #326 first and then it would be best to rebase this PR to run CI properly and check whether it passes (since it notably changes which methods the C extension defines).

[bnoordhuis]

Any idea about the musl failures

There's no musl-on-arm64 libv8-node release, as far as I can tell, never has been: https://rubygems.org/gems/libv8-node/versions

The x86_64-linux-musl builds work okay. I'll open a pull request to exclude arm64+musl from the CI matrix in .github/workflows/ci.yml

(Also affects other pull requests, FWIW)

[eregon]

There's no musl-on-arm64 libv8-node release, as far as I can tell, never has been: https://rubygems.org/gems/libv8-node/versions

There was 22.5.1.0 July 23, 2024 aarch64-linux-musl (41.5 MB), but not for the latest release 22.7.0.4.

[bnoordhuis]

Hrm, confusing... it looked like rubyjs/libv8-node@b633599d from 2021 removed aarch64-linux-musl but I guess that means it's no longer regularly tested and needs to be published manually (?) and someone forgot to?

edit: pre-existing condition, at any rate

[tisba]

See rubyjs/libv8-node#60

[eregon]

Mmh, so this PR changed lib/mini_racer.rb and broke it for the TruffleRuby backend and CI, not very nice.
The set_flags! is just one of such cases.

The way things work is the TruffleRuby backend defines the same methods as the C extension, and so all the Ruby code in lib/mini_racer.rb is reused.
But since the PR changed what the C extension defines and didn't adapt lib/mini_racer/truffleruby.rb things are broken.

I will try to fix it, but I think it's common practice that if one is changing API in a breaking way they should adapt things to make CI pass vs just breaking things.

[eregon]

BTW is there a reason most of the logic is moved to C/C++ now instead of Ruby code?
lib/mini_racer.rb was 466 lines before this PR, 93 after (explains why so many methods missing there compared to before).
That probably means many lines C/C++ to replicate the same logic.
And this means most of that logic will be duplicated, vs shared in Ruby code before.

Also this unfortunately means more fixes are needed for the truffleruby backend, if the API of the extension stayed roughly the same (or IOW if the public API stayed mostly defined in lib/mini_racer.rb ) it would just work or with minimal changes.

[eregon]

Is this change intentional?
It seems a not necessary breaking change.

Similar for the line below.

I noticed because the truffleruby backend behaves like before:

  2) Failure:
MiniRacerTest#test_symbol_support [test/mini_racer_test.rb:851]:
Expected: "foo"
  Actual: :foo

and that seems correct (to me), and the expectation seems wrong (to me).

[bnoordhuis]

It's because Symbols are not Cloneable. I work around that by returning their string representation; it was either that or return nothing.

[eregon]

Could you somehow serialize it as a string + some marker/metadata and then create a Symbol for it on the Ruby side from that string?

In general, this seems like a bigger difference between this PR and before and truffleruby, the latter two could return any live JS object/value, but only Cloneable is more restrictive. I'm not sure what's a good way to deal with that.

[bnoordhuis]

It's something I thought about when I worked on it but the short answer is "not easily" because serialization is handled by V8.

I did add a hack for function objects (transformed into strings with hopefully unique prefixes) but I'm really not happy about that so I'd rather not do it twice.

[eregon]

I see, thanks for the details.
@SamSaffron What do you think?
Should MiniRacer return Ruby String or Symbol for JS Symbols?
And should the TruffleRuby backend return Ruby String or Symbol for JS Symbols? I can easily do either.

I think this change would be worth adding to the CHANGELOG, along with all other incompatible changes (removal of isolates, some iterators are now eagerly converted to arrays)

[eregon]

Should it be [["x", 42]] instead to keep the pairs of the original iterator?

FWIW I extended the test for clarity and this is what is the current behavior:

    expected = ["x", 42, "y", 43]
    assert_equal expected, context.eval("new Map([['x', 42], ['y', 43]]).entries()")

In JS:

> e=new Map([['x', 42], ['y', 43]]).entries()
[Map Entries] { [ 'x', 42 ], [ 'y', 43 ] }
> e.next()
{ value: [ 'x', 42 ], done: false }

> e=new Map([['x', 42], ['y', 43]]).keys()
[Map Iterator] { 'x', 'y' }
> e.next()
{ value: 'x', done: false }

[eregon]

In fact on TruffleRuby it must be [["x", 42]], as AFAIK in JS there is no way to find out if a Map iterator is an entries() iterator or a values() iterator.
And so it would be incorrect for values if the arrays are flattened one level (I add this test in #328):

    expected = [[42], [43]]
    assert_equal expected, context.eval("new Map([['x', [42]], ['y', [43]]]).values()")

So I think this is something to fix in the extension to preserve the entries arrays.

[bnoordhuis]

Should it be [["x", 42]] instead to keep the pairs of the original iterator?

If that's a "should" as in "wouldn't it be nicer if", then sure, but V8's C++ API doesn't let you retrieve it that way, and I don't want to call into JS for that because that's prone to prototype pollution.

[eregon]

I guess that's flattening behavior from PreviewEntries in

mini_racer/ext/mini_racer_extension/mini_racer_v8.cc

Lines 107 to 113 in 5fad3b6

	if (v->IsWeakMap() || v->IsWeakSet() || v->IsMapIterator() || v->IsSetIterator()) {
	bool is_key_value;
	v8::Local<v8::Array> array;
	if (v8::Object::Cast(*v)->PreviewEntries(&is_key_value).ToLocal(&array)) {
	return array;
	}
	}

?
Doesn't V8 have a way to iterate an iterator in C++?
That would allow to have the same behavior as .next() in JS (and Hash#each_pair in Ruby) which is more consistent.

[bnoordhuis]

Not in a way that doesn't have observable side effects. I can of course call .next() on the iterator object, but that could be monkey-patched and execute arbitrary JS.

If Discourse or someone else wants to sponsor it, I could add a V8 C++ API to safely create/exhaust an iterator, but V8 is that kind of project where simple things are hard so it won't be a quick 30 minute job.

[eregon]

Not in a way that doesn't have observable side effects. I can of course call .next() on the iterator object, but that could be monkey-patched and execute arbitrary JS.

If people overwrite iterator.next() they are shooting themselves, I would think we don't need to care about that. Probably no non-trivial JS program runs with such a bad monkey-patch.
So I think calling next() would make a lot of sense here, and seems the only way for both backends to have the same behavior.

If Discourse or someone else wants to sponsor it, I could add a V8 C++ API to safely create/exhaust an iterator, but V8 is that kind of project where simple things are hard so it won't be a quick 30 minute job.

Maybe it could be filed as a feature request or discussion or so? I would imagine this is hardly the only project needing to iterate iterators (from C++ with V8 API). Or maybe there is some existing somewhat hidden way to do it, asking could reveal it or what others are doing.

[bnoordhuis]

If people overwrite iterator.next() they are shooting themselves, I would think we don't need to care about that.

I designed it with security in mind. That is, I'm not worried about people taking potshots at their lower appendages as I am about adversarial input.

I've been working with V8 for 15 years now and traditionally unexpected side effects are one of the most fruitful areas for the red team, so I generally tend to err on the side of caution.

Maybe it could be filed as a feature request or discussion or so?

Over at the V8 bug tracker, you mean? Sure, go ahead; please cc me.

[eregon]

Over at the V8 bug tracker, you mean? Sure, go ahead; please cc me.

I was hoping you could file the issue based on the input above, since you clearly know better about V8 and probably already have an account there.

[bnoordhuis]

@eregon a question for you: I was wondering why you're piggybacking on mini_racer? You're not using the native code and monkey-patching internals obviously is beset with perils. Why not publish a standalone truffleruby gem?

edit: I guess you could rephrase my question as: what synergy do you get out of the current arrangement that you wouldn't have with a standalone gem?

[eregon]

what synergy do you get out of the current arrangement that you wouldn't have with a standalone gem?

That the mini_racer gem which is a common dependency for gems & applications just works on TruffleRuby.
It seems very difficult or maybe even impossible to actually use libv8 as backend on TruffleRuby.

and monkey-patching internals obviously is beset with perils.

There is no monkey-patching as explained before. It's just another backend, the API of the backend is whatever methods the C extension defines. The fact that the methods the C extension defines changed caused a bit more work, but it's not too bad.