1969435 - Remove GC things from JitcodeGlobalEntry

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

This is an idea Iain had after we talked about the profiler/SM issues. Right now, JitcodeGlobalEntry holds direct references to GC-managed objects like JSScript*, and those get accessed from the profiler's sampling thread. That’s a problem because compacting GC can move things around, and we risk crashes like the one in bug 1833295, where the sampling thread tries to dereference a pointer that's in the middle of being moved.

There’s another issue too: since entries hold on to things like scripts and their realms, they can end up keeping memory alive longer than needed, especially if profiler samples are still referencing them. That can lead to shutdown crashes and memory retention.

To fix this, we should remove all GC references from JitcodeGlobalEntry. The first step is to delete the unused callStackAtAddr variant, which already relies on accessing the script. That patch is approved. Next, we can store the realmId directly in the entry at creation time so we don’t need to touch the script or realm later. After that, removing the JSScript* becomes a straightforward cleanup.

Lastly, we should make the JitCode pointer permanently weak. Instead of keeping it alive in markIteratively, we should move that logic to traceWeak, and decide whether to drop the entry or defer cleanup based on profiler state. With these changes, we avoid touching GC-managed memory off-thread and hopefully eliminate a whole class of lifetime bugs and crashes.

Comment 1

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Currently this is blocked by bug 1963824. There is an approved patch but we can't land it due to conflicts. I might rebase and land it if we don't hear from our contributor by the next week.

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Hey Iain, bug 1963824 is landed now, so hopefully there is no blocker left for this! Do you have some time to look at it? Thanks!

Comment 3

[Iain Ireland [:iain]]

I took a quick look at this yesterday. The patch to storeOne obstacle that we hadn't spotted earlier is this code in JSJitProfilingFrameIterator::tryInitWithTable, which uses the script pointer in the baseline/ion entries to filter out certain cases where the lastProfilingCallSite may not be reliable. I don't know the tryInitWithTable code very well, so I'll have to do a bit of investigation to figure out whether it's okay to stop filtering, or whether we should find a way to keep filtering without keeping scripts alive.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1969435: Store realmID directly in JitcodeGlobalEntry

This should fix bug 1833295.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1969435: Store ScriptSourceAndExtent in IonEntry/BaselineEntry instead of JSScript

Comment 6

[Iain Ireland [:iain]]

Putting up my WIP patches before going on leave. They are green on try, but I otherwise haven't tested them.

In the action plan in this doc, these patches cover parts 2-3 of the first task, and part 1 of the second task. I think this should unblock Nazim's work on JS source code in profiles. The one missing piece on my end is part 4 of the first task, which will also prevent dead JitCode from being kept alive.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3bac541d1839
https://hg.mozilla.org/integration/autoland/rev/b8109fd7b0d6
Store realmID directly in JitcodeGlobalEntry r=jandem
https://github.com/mozilla-firefox/firefox/commit/23c72626926d
https://hg.mozilla.org/integration/autoland/rev/873f721bfa65
Store ScriptSourceAndExtent in IonEntry/BaselineEntry instead of JSScript r=jandem

Comment 8

[[e-mail]]

https://hg.mozilla.org/mozilla-central/rev/b8109fd7b0d6
https://hg.mozilla.org/mozilla-central/rev/873f721bfa65