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

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

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.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to implement described changes.

Comment 2

[Terrence Cole [: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?

Comment 3

[Niko Matsakis [:nmatsakis]]

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.

Comment 4

[Terrence Cole [:terrence]]

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?

Comment 5

[Niko Matsakis [:nmatsakis]]

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.

Comment 6

[Niko Matsakis [:nmatsakis]]

I would not expect a getenv to be a performance hit either, I don't think that was the concern.

Comment 7

[Terrence Cole [:terrence]]

Do you remember what the concern was then?

Comment 8

[Niko Matsakis [:nmatsakis]]

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.

Comment 9

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to implement described changes.

Comment 10

[Terrence Cole [: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.

Comment 11

[Niko Matsakis [:nmatsakis]]

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

Comment 12

[Niko Matsakis [:nmatsakis]]

Per Terrence suggestions, I'm not going to make this change after all.

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 716025 [details] [diff] [review]
Patch to implement described changes.

Unflagging review to get it out of my queue.