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

Update to libv8-node 19.x #283

Merged
merged 7 commits into from Apr 22, 2024
Merged

Update to libv8-node 19.x #283

merged 7 commits into from Apr 22, 2024

Conversation

lloeki
Copy link
Collaborator

@lloeki lloeki commented May 27, 2023

No description provided.

This was referenced May 27, 2023
@Fayti1703
Copy link
Contributor

Not sure if this is one issue or two issues, but that looks like a problem with managing V8 locks...
Going to try and debug this in a bit.

@lloeki
Copy link
Collaborator Author

lloeki commented May 30, 2023

Awesome, thanks @Fayti1703

@tisba
Copy link
Collaborator

tisba commented May 30, 2023

Still WIP, but looks like we're changing the requirements for compiling mini_racer. If that's the case, we should probably also update the README (https://github.com/rubyjs/mini_racer#installation), which currently mentions:

Note using v8.h and compiling MiniRacer requires a C++11 standard compiler, more specifically clang 3.5 (or later) or GCC 6.3 (or later).

I was also wondering if we should add a section to the README regarding supported macOS versions. I think @lloeki mentioned somewhere that the minimum supported macOS version changed with some v8 version.

@Fayti1703
Copy link
Contributor

Fayti1703 commented Jun 1, 2023

I appear to be having a task-dependency issue when compiling locally:

Circular dependency detected:
TOP =>
compile =>
compile:x86_64-linux-gnu =>
compile:mini_racer_loader:x86_64-linux-gnu =>
copy:mini_racer_loader:x86_64-linux-gnu:3.0.2 =>
tmp/x86_64-linux-gnu/stage/lib/mini_racer_extension.so =>
lib/mini_racer_extension.so =>
copy:mini_racer_extension:x86_64-linux-gnu:3.0.2 =>
tmp/x86_64-linux-gnu/stage/lib/mini_racer_loader.so =>
copy:mini_racer_loader:x86_64-linux-gnu:3.0.2

This also occurs with v0.8.0, but not with v0.6.4.
(using Bundler version 2.3.22 & ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux-gnu])

Going to try running a bisect, see if I can track down what exactly the problem is here.

@Fayti1703
Copy link
Contributor

Fayti1703 commented Jun 1, 2023

Looks to be caused by 657a6ff...?
(That is, my problem; not the v8 abort)

@Fayti1703
Copy link
Contributor

I've managed to pin down two issues here:

  • v8::platform::PumpMessageLoop requires the Isolate to be locked.
  • Something deep in v8 is segfaulting when trying to read a snapshot -- specifically the constructor of v8::internal::StringTableInsertionKey. Unsure what the cause is at the moment.
    • crash in test_snapshots_can_be_warmed_up_with_no_side_effects
    • crash in test_isolates_from_snapshot_dont_get_corrupted_if_the_snapshot_gets_warmed_up_or_GCed

@Fayti1703
Copy link
Contributor

With a debug build of V8 (via a local libv8-node gem build), I get even more snapshot-related crashes due to invalid checksums. V8 says (for example):

#
# Fatal error in ../deps/v8/src/heap/read-only-spaces.cc, line 71
# Check failed: read_only_blob_checksum_ == snapshot_checksum (<unprintable> vs. 3847049538).
#

This smells like a V8 bug, since as far as I can tell MR doesn't touch the snapshot data whatsoever.

@lloeki
Copy link
Collaborator Author

lloeki commented Jul 4, 2023

This smells like a V8 bug

@Fayti1703 Did you by any chance attempt anything with node 20?

@Fayti1703
Copy link
Contributor

Not yet -- though judging from the CI runs on #284, there's similar behavior on that version as well.

I was planning on dumping out the checksum data on snapshot generation, trying to see if anything was off there (which would basically confirm it being a V8 issue), but I haven't found the time to do so yet.

@lloeki
Copy link
Collaborator Author

lloeki commented Sep 5, 2023

A few weeks ago I made a half-hearted attempt to trace things using GDB reverse debugging time travel, but no dice.

Half-hearted because I was not very hopeful as GDB RD is quite limited + my RD skills are limited. A more potent RD is rr, but I'm not well versed with it, although I'm a bit more hopeful: when I got stuck with GDB RD falling short, someone managed to find stuff with rr.

Maybe it's time I ramp my RD+rr game up.

@tisba
Copy link
Collaborator

tisba commented Sep 5, 2023

Out of curiosity: by rr you are referring to https://github.com/rr-debugger/rr?

@lloeki
Copy link
Collaborator Author

lloeki commented Sep 5, 2023

Yup, some vids are down below: https://rr-project.org

VERSION = "0.8.0"
LIBV8_NODE_VERSION = "~> 18.16.0.0"
VERSION = "0.9.0"
LIBV8_NODE_VERSION = "~> 19.9.0.0"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should skip non-LTS ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can find some more context to the plan we made a while ago here, @mathieujobin: #277

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that indeed explains a lot.

Makes a lot of sense.

looking forward testing 0.10.0 with node20

@lloeki
Copy link
Collaborator Author

lloeki commented Apr 5, 2024

I've moved to attempt things straight on node 20 here: #284

lloeki and others added 6 commits April 22, 2024 21:22
Not sure when this became required. Docs do say
> The caller has to make sure that this is called from the right thread.
but make no mention of required locking.
@lloeki lloeki marked this pull request as ready for review April 22, 2024 19:53
@lloeki
Copy link
Collaborator Author

lloeki commented Apr 22, 2024

Merging this as is as per the plan.

@lloeki lloeki merged commit 07acaee into main Apr 22, 2024
18 of 21 checks passed
@lloeki lloeki deleted the libv8-node-19 branch April 22, 2024 20:09
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 this pull request may close these issues.

None yet

4 participants