The browser hang with 0 CPU load.

[__Sandra__]

After the latest commits, rare browser hangs began to appear without loading the processor. I came across a link that reproduces the hang with 100% confidence for me.
Here it is: https://www.tiktok.com/@moments3641/vid ... 2882901270
Reproduced the first time you launch the browser on a clean profile.

[__Sandra__]

I have a feeling this happened after this commit https://repo.palemoon.org/MoonchildProd ... d7a5a70a9f but I haven't tried to look into it further yet.

[Moonchild]

There's a follow-up commit that landed to address this for lower thread count processors.

[__Sandra__]

There's a follow-up commit that landed to address this for lower thread count processors.

What I experience is not a decrease in performance, but a complete stop of the browser (stops responding forever). I don't think reducing the number of threads will solve this problem.

[Moonchild]

Yeah that sounds like a code deadlock, but I thought the commit you mentioned had changes to prevent that, so I'm a bit confused (and I can't reproduce this myself, so, not sure what's going on on your end?)

[__Sandra__]

I don’t know how to properly debug this situation, but when I press pause when the browser is frozen, it always shows this part of the code.

[Внешний код]
> xul.dll!base::WaitableEvent::TimedWait(const base::TimeDelta & max_time) Строка 53 C++
xul.dll!base::MessagePumpDefault::Run(base::MessagePump::Delegate * delegate) Строка 73 C++
[Внедренный фрейм] xul.dll!MessageLoop::RunInternal() Строка 228 C++
xul.dll!MessageLoop::RunHandler() Строка 222 C++
[Внедренный фрейм] xul.dll!MessageLoop::Run() Строка 201 C++
xul.dll!base::Thread::ThreadMain() Строка 179 C++
xul.dll!`anonymous namespace'::ThreadFunc(void * closure) Строка 28 C++
[Внешний код]


[Внешний код]
xul.dll!base::WaitableEvent::TimedWait(const base::TimeDelta & max_time) Строка 53
в c:\pm_src\platform\ipc\chromium\src\base\waitable_event_win.cc(53)
xul.dll!base::MessagePumpDefault::Run(base::MessagePump::Delegate * delegate) Строка 73
в c:\pm_src\platform\ipc\chromium\src\base\message_pump_default.cc(73)
[Внедренный фрейм] xul.dll!MessageLoop::RunInternal() Строка 228
в c:\pm_src\platform\ipc\chromium\src\base\message_loop.cc(228)
xul.dll!MessageLoop::RunHandler() Строка 222
в c:\pm_src\platform\ipc\chromium\src\base\message_loop.cc(222)
[Внедренный фрейм] xul.dll!MessageLoop::Run() Строка 201
в c:\pm_src\platform\ipc\chromium\src\base\message_loop.cc(201)
xul.dll!base::Thread::ThreadMain() Строка 179
в c:\pm_src\platform\ipc\chromium\src\base\thread.cc(179)
xul.dll!`anonymous namespace'::ThreadFunc(void * closure) Строка 28
в c:\pm_src\platform\ipc\chromium\src\base\platform_thread_win.cc(28)
[Внешний код]

V2.png

[athenian200]

That's interesting, it's hitting the Chromium IPC code:

https://repo.palemoon.org/MoonchildProd ... ent_win.cc

We haven't touched that code since forking off from Firefox. I remember updating some of the locking code near here ages ago, apparently in Issue #2003... wonder if maybe something in there is getting a bit long in the tooth and holding us back from taking changes we should be able to take?

Still kind of wondering why it's called here, though, I thought the Chromium IPC was used mostly for interprocess communication. So now I'm admittedly wondering if this is really called from JS directory, or if it's one of those tools I know is linked into Chromium IPC that gives us more info about how memory is used that's crashing out here? Like yeah, I know it got there... just wondering what called into it.

[Moonchild]

It might be a red herring. That's part of the main event loop driving the browser and it tends to be the "idle" handler when nothing is happening on the main thread; you would want the browser to yield to the O.S. in that case and wait.

[athenian200]

I have managed to reproduce the issue, but it's worth reiterating that it requires a fairly specific setup:

I came across a link that reproduces the hang with 100% confidence for me.
Here it is: https://www.tiktok.com/@moments3641/vid ... 2882901270
Reproduced the first time you launch the browser on a clean profile.

In my testing, it requires this exact setup. Create a new profile, go to that link. If you have launched the profile even once before, you will not be able to reproduce. Even if it crashes on first launch and you relaunch with the same profile, everything will just work the second time around. You have to use a previously unused profile to hit the issue again. In my local builds, I just deleted the obj*/tmp/scratch-user folder over and over and this was reproduceable if I remember to do that. It also seems to hit harder on a 32-bit build, though I was able to get the 64-bit one to freeze with this exact setup.

Yes, I know the OP mentioned it... but it's easy to miss if you're skim reading. I wasn't able to reproduce until I did it in this exact way. Tried once or twice before noticing the words I just bolded. That seems to hint at something that's not setup yet in a new profile causing a hard crash, actually. At least for this specific link.

[athenian200]

So... good news and bad news. Reverting this commit fixes it:

https://repo.palemoon.org/MoonchildProd ... 0c6db7c47b

That is, I cannot reproduce whatever is causing this crash anymore if I revert the commit that stops limiting parse threads to 1. So while the rest of Basilisk-Dev's changes appear innocent here, whatever he did just wasn't quite enough to make it 100% safe for us to use more than one parse thread. Mozilla probably assumed it was just the WASM stuff, but may have not noticed everything else that had to change to make it safe to do that.

It's kind of a shame, because it works most of the time... but in a few specific situations, like being on a fresh profile with a very busy web page, it crashes. So the good news is we can stop it from happening... the bad news is we are stuck with a single-threaded parser which is not ideal...

[athenian200]

Okay, so I did some more experimenting with different values now that I can reproduce the issue consistently on my quad-core i7-3770.

What I found was that using just using threadCount freezes it every time. Using a value of two didn't. But then I got the weird idea from somewhere in the back of my mind to try threadCount - 1. It worked consistently even just subtracting 1 from the total thread count. So, if my intuition based on these observations is right, the problem with just using threadCount directly is that it uses ALL threads that are available at once in some cases, and doesn't leave anything free.

[Moonchild]

So, if my intuition based on these observations is right, the problem with just using threadCount directly is that it uses ALL threads that are available at once in some cases, and doesn't leave anything free.

Funny, I was considering the exact same.
https://repo.palemoon.org/MoonchildProd ... ment-46821

So we should probably just reserve threads so we aren't deadlocking the kernel. This is likely a subtlety of any threading model because I've seen the same kind of thing with pthreads on Linux.

[Moonchild]

I landed a commit on master to reduce the thread count by 1. If still problematic, we should probably just reserve 2 threads (i.e. 1 core worth in case of HT) to keep parser threads reduces sufficiently. There are more classes of helper threads after all, so it's conceivable just reserving 1 thread isn't enough for corner cases.

[Basilisk-Dev]

I guess that's why Mozilla and Seamonkey used cpuCount instead of threadCount

[__Sandra__]

I have managed to reproduce the issue, but it's worth reiterating that it requires a fairly specific setup:

I managed to write this condition by accident. When I wrote it, I meant that the freeze occurs on a completely clean profile without the intervention of any additional settings.

Most likely it is the overall performance of the system that matters. On my systems with Intel Core i3-2330M and Intel Core i5-2435M, freezes also occurred during normal browsing.

[__Sandra__]

So we should probably just reserve threads so we aren't deadlocking the kernel. This is likely a subtlety of any threading model because I've seen the same kind of thing with pthreads on Linux.

If I understand correctly how this works, we add 4 to the number of physical (logical?) cores and get the number of threads for variable "threadCount".

Code: Select all

static size_t
ThreadCountForCPUCount(size_t cpuCount)
{
    // Create additional threads on top of the number of cores available, to
    // provide some excess capacity in case threads pause each other.
    static const uint32_t EXCESS_THREADS = 4;
    return cpuCount + EXCESS_THREADS;
}

This calculation gives a disproportionate increase in threads to the number of cores. On a modern processor with 12 cores, we will get 33% (16 versus 12) more threads than cores, and on a processor with 4 cores, the increase in threads is 100% (8 versus 4). Maybe it’s worth using a different calculation principle, for example, doubling the number of threads per core?

Code: Select all

static size_t
ThreadCountForCPUCount(size_t cpuCount)
{
    // Create additional threads on top of the number of cores available, to
    // provide some excess capacity in case threads pause each other.
    static const uint32_t EXCESS_THREADS = 2;
    return cpuCount * EXCESS_THREADS;
}

[Moonchild]

Good catch. I had not actually looked at the code but just adding 4 to core count is... well really dumb. You'd be under-utilizing high core count CPUs and over-stressing low core count CPUs... Definitely should be adjusted. In fact, if we still run into capacity issues, we might use a lower ratio than 2 and floor it and we won't have to make any other changes at callsites.

[athenian200]

This calculation gives a disproportionate increase in threads to the number of cores. On a modern processor with 12 cores, we will get 33% (16 versus 12) more threads than cores, and on a processor with 4 cores, the increase in threads is 100% (8 versus 4). Maybe it’s worth using a different calculation principle, for example, doubling the number of threads per core?

Well, actually I turned up something interesting here.

https://learn.microsoft.com/en-us/windo ... ystem_info

https://repo.palemoon.org/MoonchildProd ... .cpp#L3072

Essentially, if I'm reading the code correctly, the function called to define cpuCount actually calls the sysinfo function that returns the number of logical processors, not the number of physical CPU cores like we were all thinking. So, at least on Windows for sure, cpuCount would actually be 8 on my system, not 4. Which... makes the function name kinda misleading. You'd think they'd call it "logicalProcessorCount" or something similar to clarify that it's not the number of CPUs, but the number of execution contexts that is getting reported here. Which of course means that threadCount doesn't refer to anything meaningful, other than just being a very unfortunate name especially next to cpuCount, since it just refers to an arbitrary number of software threads that have absolutely nothing to do with "threads" in the CPU sense.

If this is right... does that mean this was using 12 parse threads before my old quad core started having an application freeze with a fresh profile, and backing off to 11 was what made it rock solid stable for me? It was definitely handling more parse threads than the number of logical processors would imply, in that case. This was the only function in the codebase with the same name as what it was calling, so I assume it was this one... though it might actually pay to see if we can get it to tell us what it's reporting for cpuCount here with some kind of print statement or logging just to make sure my analysis is correct.

[Moonchild]

Oh jeez.

Well in that case, we just need to go back to what was there initially, and just use cpuCount for parse threads.

[Moonchild]

Reverted the changes and ended up doing it this way, which should work fine on all hardware:

Code: Select all

static size_t
ThreadCountForCPUCount(size_t cpuCount)
{
    // Create additional threads on top of the number of cores available, to
    // provide some excess capacity in case threads pause each other.
    // Note that cpuCount here is the number of logical processors and threadCount
    // calculated here just adds some extra capacity on top. Use threadCount
    // with care as it may end up deadlocking.
    static const uint32_t EXCESS_THREADS = 4;
    return cpuCount + EXCESS_THREADS;
}

{...}

size_t
GlobalHelperThreadState::maxParseThreads() const
{
    if (IsHelperThreadSimulatingOOM(js::oom::THREAD_TYPE_PARSE))
        return 1;
    // Use the number of logical processors in a system.
    return cpuCount;
}