Skip to content

Remove PThread.preallocatedWorkers#10269

Merged
juj merged 3 commits intoemscripten-core:masterfrom
juj:PTHREAD_POOL_DELAY_LOAD
Feb 4, 2020
Merged

Remove PThread.preallocatedWorkers#10269
juj merged 3 commits intoemscripten-core:masterfrom
juj:PTHREAD_POOL_DELAY_LOAD

Conversation

@juj
Copy link
Copy Markdown
Collaborator

@juj juj commented Jan 23, 2020

When reviewing #10263 I noticed there is a new setting PTHREAD_POOL_DELAY_LOAD that has popped into existence in #9394.

This PR simplifies the implementation of that setting by refactoring not to need a preallocatedWorkers array.

@juj
Copy link
Copy Markdown
Collaborator Author

juj commented Jan 23, 2020

Updated this PR to remove the PThread.preallocatedWorkers concept to simplify the code.

@juj juj changed the title PTHREAD_POOL_DELAY_LOAD was redundant Remove PThread.preallocatedWorkers Jan 23, 2020
@juj
Copy link
Copy Markdown
Collaborator Author

juj commented Jan 29, 2020

Ping on this one. I think this should be easy to land - it simplifies code to remove unnecessary double management of preallocatedWorkers vs unusedWorkers. and the splitting of functions that came with that. It looks perhaps bigger change than it actually is because it moves a large function around.

Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable, and I really appreciate the improved documentation of these settings.

I'll leave it to @kripken to give the final approval

#include <emscripten.h>
#include <emscripten/threading.h>
#include <assert.h>
#include <stdio.h>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is to enable that file to build outside the test harness. The test harness adds implicit stdio.h include somewhere in the added with_report_result concatenation or something like that - the file won't build locally otherwise.

@kripken
Copy link
Copy Markdown
Member

kripken commented Jan 29, 2020

ping @belraquib - hoping you can chime in here, to understand the issues @juj raised about #9394

@juj juj force-pushed the PTHREAD_POOL_DELAY_LOAD branch from 9a0da02 to 7766d6f Compare January 30, 2020 05:07
@juj
Copy link
Copy Markdown
Collaborator Author

juj commented Feb 3, 2020

Any chance this could proceed, looks like @belraquib might not be available to review?

Copy link
Copy Markdown
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked with @belraquib offline, no concerns, lgtm. Nice refactor!

@juj juj merged commit 1f3ac3a into emscripten-core:master Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants