Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RFC - support KHR_parallel_shader_compile #16662

Closed
wants to merge 32 commits into from
Closed

RFC - support KHR_parallel_shader_compile #16662

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
573eee7
WIP demo parallel compile
aardgoose May 24, 2019
2d30ed5
WIP
aardgoose May 25, 2019
16b5607
Merge commit '2d30ed58772302420cd9a8bcb8cb3e16706168f4' of https://gi…
aardgoose May 25, 2019
cf97d60
update to latest paralle1
aardgoose May 30, 2019
e4d71a4
update to lhash
aardgoose May 30, 2019
bc79134
adjust to lhash
aardgoose May 30, 2019
85962cd
Merge branch 'lhash' into parallel2
aardgoose May 30, 2019
756ab02
make material specific
aardgoose May 30, 2019
4359c68
WIP shared programs etc
aardgoose May 30, 2019
60bce20
default to no parallel compile
aardgoose May 30, 2019
adff022
testing
aardgoose May 31, 2019
eb9c7fe
Merge branch 'dev' into parallel2
aardgoose Jun 1, 2019
e7e2d26
Merge branch 'dev' into parallel2
aardgoose Jun 2, 2019
eb44c9a
remove testing parts
aardgoose Jun 3, 2019
368e91b
add sync mode to preserve .compile() behaviour
aardgoose Jun 3, 2019
2a88ce0
limit simultaneous compiles
aardgoose Jun 4, 2019
447638f
Merge branch 'dev' into parallel2
aardgoose Jun 9, 2019
1e493fb
Merge branch 'dev' into parallel2
aardgoose Jun 23, 2019
7f462f4
merge dev
aardgoose Sep 12, 2019
6676ae3
Merge branch 'dev' into parallel2
aardgoose Sep 16, 2019
774b5c8
document capability
aardgoose Sep 16, 2019
b2f5822
doc update
aardgoose Sep 16, 2019
715f6f3
use getContext()
aardgoose Sep 16, 2019
c04a231
remove Material.parallelCompile
aardgoose Sep 16, 2019
810c9c2
move switch back to material
aardgoose Sep 16, 2019
9ac963f
fix race on destruction and tidy
aardgoose Sep 16, 2019
d23cab2
Merge branch 'dev' into parallel2
aardgoose Sep 17, 2019
25d3707
Add global default
aardgoose Sep 17, 2019
afa66ae
Merge branch 'dev' into parallel2
aardgoose Sep 20, 2019
e8ddd76
pass extension in parameters
aardgoose Sep 20, 2019
f2385ce
merge
aardgoose Sep 25, 2019
38a7aad
merge dev
aardgoose Oct 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/materials/Material.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ function Material() {
this.premultipliedAlpha = false;

this.visible = true;
this.parallelCompile = false;
Copy link
Owner

Choose a reason for hiding this comment

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

Is this boolean needed?
What are the downsides of using KHR_parallel_shader_compile by default?

Copy link
Collaborator

@takahirox takahirox Sep 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

A material can be ready a few animation frames away since the material is added to the scene. That means The mesh can't show up in the first few animation frames. So if it's true as default

  1. It breaks the compatibility.
  2. It can be problematic for applications which don't use animation frame, for example calling renderer.render() only when something in the application is changed.

Copy link
Owner

@mrdoob mrdoob Sep 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Does it need to be per material? Or can it be per renderer?

Copy link
Collaborator

@takahirox takahirox Sep 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Personally I think moving the flag to renderer would sound simpler.

Copy link
Collaborator

@takahirox takahirox Sep 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What are the downsides of using KHR_parallel_shader_compile by default?

In addition to what I mentioned above, if it hasn't been updated, there is a chance that the extension may have performance issue on Chrome due to browser side problem now. (Sorry I've been delaying to evaluate and report! I'm happy if anyone evaluates.) Even if it still has performance problem, we may go forward. Chrome may resolve sooner or later.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, may be easier to start with the renderer first, and if people need more granularity we can consider moving to deeper objects.

Copy link
Contributor Author

@aardgoose aardgoose Sep 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What is your preferred API for a renderer swicth?

A new property of the parameters object or a simple boolean parameter of the renderer itself?

I think the later is simpler, since the renderer is passed as a parameter to WebGLProgram()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I have found out an issue with a renderer wide switch.

Some passes like the PMREMGenerator carry out single pass operations which can and do fail with parallel compilation enabled when the compilation does not complete immediately, I imagine there are other cases out there.

Copy link
Owner

Choose a reason for hiding this comment

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

I see...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could set a global default on Material.DefaultParallelCompile and use that to init each material.

So a user could opt in globally for simplicity but the default would be safe for all the odd corner cases out there.


this.toneMapped = true;

Expand Down
34 changes: 25 additions & 9 deletions src/renderers/WebGLRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,8 @@ function WebGLRenderer( parameters ) {

var program = setProgram( camera, fog, material, object );

if ( ! program.ready ) return;

var updateBuffers = false;

if ( _currentGeometryProgram.geometry !== geometry.id ||
Expand Down Expand Up @@ -1042,13 +1044,13 @@ function WebGLRenderer( parameters ) {

for ( var i = 0; i < object.material.length; i ++ ) {

initMaterial( object.material[ i ], scene.fog, object );
initMaterial( object.material[ i ], scene.fog, object, true );

}

} else {

initMaterial( object.material, scene.fog, object );
initMaterial( object.material, scene.fog, object, true );

}

Expand Down Expand Up @@ -1444,6 +1446,8 @@ function WebGLRenderer( parameters ) {

var program = setProgram( camera, scene.fog, material, object );

if ( ! program.ready ) return;

_currentGeometryProgram.geometry = null;
_currentGeometryProgram.program = null;
_currentGeometryProgram.wireframe = false;
Expand All @@ -1461,7 +1465,7 @@ function WebGLRenderer( parameters ) {

}

function initMaterial( material, fog, object ) {
function initMaterial( material, fog, object, sync ) {

var materialProperties = properties.get( material );

Expand All @@ -1488,13 +1492,13 @@ function WebGLRenderer( parameters ) {
// changed glsl or parameters
releaseMaterialProgramReference( material );

} else if ( materialProperties.lightsStateVersion !== lightsStateVersion ) {
} else if ( program.ready && materialProperties.lightsStateVersion !== lightsStateVersion ) {

materialProperties.lightsStateVersion = lightsStateVersion;

programChange = false;

} else if ( parameters.shaderID !== undefined ) {
} else if ( program.ready && parameters.shaderID !== undefined ) {

// same glsl and uniform list
return;
Expand Down Expand Up @@ -1542,6 +1546,15 @@ function WebGLRenderer( parameters ) {

}

if ( ! program.ready && ! program.isLinked( _this, material, sync ) ) {

materialProperties.retry = true;
return;

}

materialProperties.retry = false;

var programAttributes = program.getAttributes();

if ( material.morphTargets ) {
Expand Down Expand Up @@ -1674,9 +1687,9 @@ function WebGLRenderer( parameters ) {

}

if ( material.needsUpdate ) {
if ( material.needsUpdate || materialProperties.retry ) {

initMaterial( material, fog, object );
initMaterial( material, fog, object, false );
material.needsUpdate = false;

}
Expand All @@ -1685,8 +1698,11 @@ function WebGLRenderer( parameters ) {
var refreshMaterial = false;
var refreshLights = false;

var program = materialProperties.program,
p_uniforms = program.getUniforms(),
var program = materialProperties.program;

if ( ! program.ready ) return false;

var p_uniforms = program.getUniforms(),
m_uniforms = materialProperties.shader.uniforms;

if ( state.useProgram( program.program ) ) {
Expand Down
5 changes: 4 additions & 1 deletion src/renderers/webgl/WebGLCapabilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ function WebGLCapabilities( gl, extensions, parameters ) {
var floatVertexTextures = vertexTextures && floatFragmentTextures;

var maxSamples = isWebGL2 ? gl.getParameter( gl.MAX_SAMPLES ) : 0;
var parallelShaderCompile = extensions.get( 'KHR_parallel_shader_compile' );

var multiviewExt = extensions.get( 'OVR_multiview2' );
var multiview = isWebGL2 && !! multiviewExt && ! gl.getContextAttributes().antialias;
Expand Down Expand Up @@ -117,7 +118,9 @@ function WebGLCapabilities( gl, extensions, parameters ) {
maxSamples: maxSamples,

multiview: multiview,
maxMultiviewViews: maxMultiviewViews
maxMultiviewViews: maxMultiviewViews,

parallelShaderCompile: parallelShaderCompile

};

Expand Down
Loading