Default onBeforeCompile=null to reduce unnecessary calls to programCache.getProgramCode()

[EliasHasle]

The price of the change is having to check for the existence of onBeforeCompile before calling it. The gain is avoiding an extra run of programCache.getProgramCode on each compile. This call may have a very small footprint compared to the typical compilation stage, but I woud expect the existence check to have an even smaller footprint. It looks like the extra programCache.getProgramCode call was added after the decision was made to have an empty function as default. I have not tested performance in any different scenarios, though. Update: I have tailored a scenario further down which demonstrates an improvement.

Also, I have not paid much consideration to the TypeScript declarations. A quick Google search tells that there is a flag for strict handling of null and undefined values, but otherwise they are valid values for anything.

[Mugen87]

For reference: The additional call of programCache.getProgramCode() was introduced with #14442

Um, it's a bit inconsistent if Object3D.onBeforeRender() and Object3D.onAfterRender() have different default values than Material.onBeforeCompile(). TBH, I tend to accept the additional call of programCache.getProgramCode() if the API stays consistent.

[EliasHasle]

Thanks for the reference.

The user API will be the same anyway, since only the renderer calls onBeforeCompile. The only practical discrepancy I can see is that the user may in some cases find it useful to check whether material.onBeforeCompile is set, and this type of test will not generalize to object.onBeforeRender (which arguably could be a bad thing).

[Mugen87]

The user API will be the same anyway,

Sorry, but that's technically not true. The default value will be different and onBeforeCompile is a public method.

[EliasHasle]

Right, but which currently possible/sensible use cases will break?

[Mugen87]

You're right, there is actually no reasonable scenario where user code might break.

I'm still a bit unsure about the resulting differences between Material.onBeforeCompile() and Object3D.onBeforeRender() and Object3D.onAfterRender(). But if @mrdoob does not share these concerns, I'm fine with this change.

[EliasHasle]

I understand. I have not presented a running example that demonstrates a significant speedup either, though I am almost sure it is possible to make one. (That would make this tiny change a somewhat larger project.)

Who do we/you know that struggles with long scene init time?

[Mugen87]

Who do we/you know that struggles with long scene init time?

Can't you just setup a scene with many different materials?

[EliasHasle]

You're right, there is actually no reasonable scenario where user code might break.

I forgot to correct the onBeforeCompile.toString() of programCache.getProgramCode in the first version (at the same time disclosing that my code changes were untested at the time). Are there current applications that use onBeforeCompile.toString()? @sunag @pailhead @donmccurdy

Can't you just setup a scene with many different materials?

Yes... I suppose so. Everything takes time, though, and I don't strictly have that. Also, when programatically generating several similar materials, I may encounter browser optimizations that invalidate some conclusions. I may give it a try soon, though. Maybe now.

[sunag]

Yes, NodeMaterial use toString but this change seems to be compatible with this and honestly I already thought of making this change... @Mugen87 We could set a default of value not set for threejs once undefined !== null is true?

[EliasHasle]

Stress test comparison that shows it is possible to get better performance with the patch:
https://eliashasle.github.io/testing/material_init_stress_status_quo.html
https://eliashasle.github.io/testing/material_init_stress_patched.html

This is a rather extreme test with 300 000 RawShaderMaterial instances made from the same extremely simple code and applied once to a shared geometry.

Yes, NodeMaterial use toString but this change seems to be compatible with this

null.toString() fails, but if you don't mind adding a check for null, it should be no problem for your case.

[sunag]

null.toString() fails

I was no referring to this approach, it is until primary... but toString method is created after a function onBeforeCompile making compatible with this code.

three.js/examples/jsm/nodes/materials/NodeMaterial.js

Lines 31 to 51 in b163e54

	this.onBeforeCompile = function ( shader, renderer ) {
	if ( this.needsUpdate ) {
	this.build( { renderer: renderer } );
	shader.uniforms = this.uniforms;
	shader.vertexShader = this.vertexShader;
	shader.fragmentShader = this.fragmentShader;
	}
	};
	// it fix the programCache and share the code with others materials
	this.onBeforeCompile.toString = function () {
	return self.needsCompile;
	};

[EliasHasle]

Oh, of course! Now I remember that I found out that yesterday anyway... Any risk that other power users have code that will break? Hm.

[Mugen87]

This PR actually became obsolete because of the refactoring of #18279. onBeforeCompile() is now called in WebGLPrograms.getParameters(). That means the callback is now always executed before getProgramCacheKey() (previously getProgramCode()) is called.

So an additional invocation of getProgramCacheKey() does not happen anymore. Thus, an existence check for onBeforeCompile() is not necessary.

[pailhead]

Just curious, i'd be interested if the empty function was referenced from a single noop instance with a strict equality check to compare the references. I think previous benchmarks shown that it's more effecient to just run a noop than to do a null check, but i agree that it makes no sense to invalidate the cache basically because this function just gets fired.

I think the way it's consolidated in @Mugen87 PR is a better fix for this. I'm interested in what toString() could be overridden to, and if it could be used to manage caching better but i'm not sure what's the state of that, and it's not a topic here.

But yeah, if this were still valid, without the other PR, just from a JS standpoint, i think things could be kept consistent (without .func = null by referencing a common noop.