The default bug view has changed. See this FAQ.

Reduce video thread stack size, on 32-bit linux at least

RESOLVED FIXED in mozilla8

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: cpearce)

Tracking

unspecified
mozilla8
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
From bug 592833 comment 9:

> Reducing the thread stack size is a good idea, as we reserve 10MB per thread
> stack on Linux, which kills us on 32bit Linux. IIRC, we use 1MB thread stacks
> on Windows and OSX, so the problem isn't so pronounced there, but we can
> probably still make some easy gains by reducing stack size there anyway.
(Reporter)

Updated

6 years ago
Whiteboard: [MemShrink:P2]
How deep do these threads get?  If they don't use much of the stack we should shrink windows too ... 1 MB * 3 threads * N audio/video elements is a fair amount of virtual address space.  I've seen minidumps with 100s of AV threads.
Linux is 8MB by default, not 10MB.  OS X uses 512kB by default and we haven't seen stack exhaustion there, so we could probably reduce Win32 thread stack sizes safely.

Fixing this will require some new API for nsIThread/NS_NewThread.
libvorbis uses alloca, potentially of several times the largest block size (which is 8192). libtheora requires less than 2kB of stack. I'd need to double-check for libvpx, but I would expect this to be similarly small.
(Reporter)

Comment 4

6 years ago
(In reply to comment #1)
> 1 MB * 3 threads * N audio/video elements

Bug 592833 will avoid the "* 3" term.
(Reporter)

Comment 5

6 years ago
Just to clarify:  we'd only do this for audio/video threads, right?
(Assignee)

Comment 6

6 years ago
Why does Linux need such large stack sizes compared to other platforms in general? bent/cjones?
Summary: Reduce thread stack size, on 32-bit linux at least → Reduce video thread stack size, on 32-bit linux at least
(In reply to comment #6)
> Why does Linux need such large stack sizes compared to other platforms in
> general? bent/cjones?

I don't think it needs them ... that's just the default.
Right.  Threads' stack memory isn't committed on alloc, so a large default usually makes sense.
(Assignee)

Updated

6 years ago
Assignee: nobody → chris
Status: NEW → ASSIGNED
(In reply to comment #8)
8M seems excessive though.  Perhaps we should institute a 1M limit
for all stacks on all platforms, and try to stay within (eg) 256k
so as to leave a good safety margin.  If the Linux kernel can get
by in an 8K stack, 8M seems wildly extravagant.

gcc-4.6.0 at least has a 

  -Wframe-larger-than=<number> Warn if a function's stack frame requires more
                              than <number> bytes

which might be useful.
(Assignee)

Comment 10

6 years ago
Created attachment 541516 [details] [diff] [review]
Patch: Add API to specify amount reserved for thread stack size

Add an API nsIThreadManager and nsThread to specify the size of thread stacks. This just passes the value through to PR_CreateThread(), which already allows you to set stack size.

I have another patch to set the size of thread stacks in the <video>, but I'll wait until this patch gets r+ before posting that.
Attachment #541516 - Flags: review?(benjamin)
(Assignee)

Comment 11

6 years ago
bsmedberg: review ping?
Comment on attachment 541516 [details] [diff] [review]
Patch: Add API to specify amount reserved for thread stack size

JS shouldn't be using this API, but you will at least need to fix up test_bug608142.js

I'd really like a constant for the stack size, instead of hardcoded 0 (e.g. const unsigned int DEFAULT_STACK_SIZE = 0; in nsIThreadManager)

And can you just add a default argument to the existing NS_NewThread instead of declaring another version?

r=me with at least the constant
Attachment #541516 - Flags: review?(benjamin) → review+
(Assignee)

Comment 13

6 years ago
Created attachment 548350 [details] [diff] [review]
Patch v2: Add API to specify amount reserved for thread stack size

With bmsedberg's review comments addressed. Carrying forward r=bsmedberg.

I made the stack size argument in nsIThreadManager [optional], so that existing js which uses this API (in our tests) don't need to change. I also added the constant DEFAULT_STACK_SIZE in nsIThreadManager as requested.
Attachment #548350 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #541516 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
Created attachment 548632 [details] [diff] [review]
Patch: Specify media threads' stack size

Specify the media decoder's threads' stacks to reserve 128KB. I found that on my x64 Linux box I needed at least 98KB thread stacks when running mochitests with a debug build (opt builds may use less), so I rounded stack size up to 128KB to give us some wiggle room.
Attachment #548632 - Flags: review?(kinetik)
Comment on attachment 548632 [details] [diff] [review]
Patch: Specify media threads' stack size

Review of attachment 548632 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/VideoUtils.h
@@ +157,5 @@
>  // before being used!
>  void ScaleDisplayByAspectRatio(nsIntSize& aDisplay, float aAspectRatio);
>  
> +// The amount of virtual memory reserved for thread stacks.
> +#if defined(XP_WIN) || defined(XP_MACOSX) || defined(LINUX)

XP_LINUX?
Attachment #548632 - Flags: review?(kinetik) → review+
(Assignee)

Comment 16

6 years ago
Nice catch, thanks. MEDIA_THREAD_STACK_SIZE should be nsIThreadManager::DEFAULT_STACK_SIZE too, not 0.
(Assignee)

Comment 17

6 years ago
Ooops, XP_LINUX isn't actually defined centrally:
http://mxr.mozilla.org/mozilla-central/search?string=XP_LINUX
Have to go with using defined(LINUX)
(Assignee)

Comment 18

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/c56067ea7988
http://hg.mozilla.org/integration/mozilla-inbound/rev/685b3762558c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/c56067ea7988
http://hg.mozilla.org/mozilla-central/rev/685b3762558c

please don't mark as fixed till bugs are merged to central.
You need to log in before you can comment on or make changes to this bug.