Make number of threads used in threadpool configurable even in opt builds

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: nmatsakis, Unassigned)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Currently the number of threads used by vm/ThreadPool.cpp can only be configured in debug builds.  This patch adds a configure option (--enable-manual-threads) that can be used with opt builds to enable the JSTHREADS environment variable.
(Reporter)

Comment 1

6 years ago
Created attachment 715669 [details] [diff] [review]
Patch to implement described changes.
Attachment #715669 - Flags: review?(terrence)
Comment on attachment 715669 [details] [diff] [review]
Patch to implement described changes.

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

This seems like total overkill to me. This seems like a really small tweak to merit adding yet another configure option to our already massive list. Also, could we call the environment variable something more descriptive than JSTHREADS? How about JS_THREADPOOL_SIZE?
Attachment #715669 - Flags: review?(terrence)
(Reporter)

Updated

6 years ago
Blocks: 829602
(Reporter)

Comment 3

6 years ago
Shall I just always check the environment variable? even in no opt builds? Alternatively, is there a pre-existing configure option I can make use of?

Also, I will change the name of the environment variable to JS_THREADPOOL_SIZE.

When I initially introduced the thread pool, I believe dmandelin suggested that it would not be a good idea to always check the environment variable, which is why I added this switch, but I'm just as happy to always check the variable.
I wouldn't expect a single getenv to regress our startup that badly, however, dmandelin would know better than I would so we should find a better way. What is your purpose here? Would a flag to the shell work? Could it just live under the rivertrail #ifdef?
(Reporter)

Comment 5

6 years ago
There is no rivertrail ifdef.  The purpose is for use when benchmarking and debugging.  For benchmarking it's useful to observe scalability.  For debugging it's sometimes useful to have less parallelism.
(Reporter)

Comment 6

6 years ago
I would not expect a getenv to be a performance hit either, I don't think that was the concern.
Do you remember what the concern was then?
(Reporter)

Comment 8

6 years ago
It was bug 801087.  Reviewing the quote, perhaps I was wrong to say that dmandelin was opposed.  He wrote the following:

> ::: js/src/jsthreadpool.cpp
> @@ +205,5 @@
> > +ThreadPool::init()
> > +{
> > +    // Compute desired number of workers based on env var or # of CPUs.
> > +    size_t numWorkers = 0;
> > +    char *pathreads = getenv("PATHREADS");
> 
> Is this for development testing purposes? An environment variable is OK for that > purpose (although normal options would be better) if that's clearly indicated.

So perhaps I'll just update the name of the environment variable and remove the configure option.
(Reporter)

Comment 9

6 years ago
Created attachment 716025 [details] [diff] [review]
Patch to implement described changes.
Attachment #715669 - Attachment is obsolete: true
Attachment #716025 - Flags: review?(terrence)
Actually, why do we need to carry this on inbound at all? If you just want it for testing then it makes much more sense to just carry a change to remove the #ifdefs locally.
(Reporter)

Comment 11

6 years ago
Sure, I guess that's an option too. I can leave in #ifdef DEBUG and people can manually comment that out if necessary.
(Reporter)

Comment 12

6 years ago
Per Terrence suggestions, I'm not going to make this change after all.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
Comment on attachment 716025 [details] [diff] [review]
Patch to implement described changes.

Unflagging review to get it out of my queue.
Attachment #716025 - Flags: review?(terrence)
(Reporter)

Updated

6 years ago
No longer blocks: 829602
You need to log in before you can comment on or make changes to this bug.