Three.js: WebGLLights: Inefficient string concatenations

[oguzeroglu]

https://github.com/mrdoob/three.js/blob/8c6d24b0876f0f7d23aa60be55d6570f4ddb4d6b/src/renderers/webgl/WebGLLights.js#L319

Related PR: https://github.com/mrdoob/three.js/pull/14568

This line is executed 60 times per second thus produces garbage as seen on Chrome's Allocation Profiler:

The hash value of WebGLLights should be an object instead of concatanated string:

hash: {
stateID: -1,
directionalLength: -1,
pointLength: -1,
spotLength: -1,
rectAreaLength: -1,
hemiLength: -1,
shadowsLength: -1
}

In the WebGLRenderer.js the values should be compared as:

} else if ( materialProperties.lightsHash.stateID !== lights.state.hash.stateID ||
        materialProperties.lightsHash.directionalLength !== lights.state.hash.directionalLength ||
    materialProperties.lightsHash.pointLength !== lights.state.hash.pointLength ||
        materialProperties.lightsHash.spotLength !== lights.state.hash.spotLength ||
    materialProperties.lightsHash.rectAreaLength !== lights.state.hash.rectAreaLength ||
    materialProperties.lightsHash.hemiLength !== lights.state.hash.hemiLength ||
    materialProperties.lightsHash.shadowsLength !== lights.state.hash.shadowsLength ) {

Since JS assigns objects by reference materialProperties.lightsHash should be updated like this to avoid potenial bugs caused by string to object switch:

materialProperties.lightsHash.stateID = lights.state.hash.stateID;
materialProperties.lightsHash.directionalLength = lights.state.hash.directionalLength;
materialProperties.lightsHash.pointLength = lights.state.hash.pointLength;
materialProperties.lightsHash.spotLength = lights.state.hash.spotLength;
materialProperties.lightsHash.rectAreaLength = lights.state.hash.rectAreaLength;
materialProperties.lightsHash.hemiLength = lights.state.hash.hemiLength;
materialProperties.lightsHash.shadowsLength = lights.state.hash.shadowsLength;

instead of:

materialProperties.lightsHash = lights.state.hash

Any ideas/opinions?

[Mugen87]

I think it's only consequent to proceed with the refactoring introduced in #14568.

[mrdoob]

Yep! Look good to me 👌

[Usnul]

That's not a hash, a surrogate - maybe.

In general, what is called "hash" here takes a only a subset of data, but it is not a real indicator of equivalence. It's just a hack. Maybe rename the property from state.hash to state.hacksh for clarity while we're at it.

This is certainly a tangent, but I think we're talking about how to grease this 3rd square wheel on a bicycle instead of figuring out why it's there in the first place.

[oguzeroglu]

It is there to prevent redundant webgl calls/material updates as I understood from the code and it makes sense. Yes it is not a real hash but it produces a unique value from given values so it kind of works like a hash anyway, also much cheaper than producing a real hash inside the render loop in my opinion.

[oguzeroglu]

@Mugen87 could you please specify which refactoring are you talking about? There are loads of suggested models in that PR :)

[oguzeroglu]

@Usnul

In my comment there was an error:

It is there to prevent redundant webgl calls/material updates as I understood from the code and it makes sense.

The hash value is used in WebGLRenderer. In this line, if I understood correctly when there's a new light on the scene and the user did not mark materials as needsUpdate = true, it refreshes the material so that the effect of the light can be seen:
https://github.com/mrdoob/three.js/blob/8c6d24b0876f0f7d23aa60be55d6570f4ddb4d6b/src/renderers/WebGLRenderer.js#L1611

In this line using the hash value, a decision whether to change the whole shader program or to update only the uniforms is given:
https://github.com/mrdoob/three.js/blob/8c6d24b0876f0f7d23aa60be55d6570f4ddb4d6b/src/renderers/WebGLRenderer.js#L1441

[Usnul]

@oguzeroglu

Yes it is not a real hash but it produces a unique value from given values so it kind of works like a hash anyway

Okay, let's say that between render calls I decide to add a new directional light, and remove one of the old directional lights. Light state has most certainly changed, but the "hash" remains the same.

This is just one example of that "uniqueness" not existing.

Conversely, if you have two, for all intents and purposes, identically lit scenes - because they are different scenes - they will have different light "state" objects associated with them, and stateID is generated in the constructor, which means that those two identical states will never produce identical "hashes", as stateID is a part of that "hash" and will be different for each instance.

also much cheaper than producing a real hash inside the render loop in my opinion.

I am not so sure. The whole point of hashes is that they are cheap to produce, unlike the thing that we do here. Here's what happens in the current implementation:
1 - read X strings
2 - generate a new object
3 - compare object with another (hash check)

"Hash" ends up as a large object, and you end up reading large strings as a result, and every time you compute hash you generate garbage.

All of the above is an anti-pattern for hash implementation, precisely for performance reasons.

Now, back to the original topic. I suggest implementing "clone" and "equals" methods on state instead. At least it won't be wrong.

[oguzeroglu]

@Usnul

You're absolutely right abot the hash concept here but this issue is about reducing the GC activity by avoiding creation of new objects in the render loop. In the solution proposed by me the objects are created only once but their properties (which are all integer values, no strings here) are changed inside the loop so it should be quite cheap considering the memory usage. If I understood your proposal correctly
a new String object would be created inside the render loop anyway when we hash couple of integers together (AFAIK you cannot reuse Strings since they are immutable objects). So your proposal is exactly what I suggest avoiding in this issue.

I'm not saying it's not a good idea or anything, it is just against what this issue proposes that's all :)

[Mugen87]

could you please specify which refactoring are you talking about?

I just refer to the avoidance of string concatenations. And yes, let's focus on this topic and not about other possible issues regarding WebGLLights. There is already #14121.

[haroldiedema]

I would like to see performance numbers when comparing string concatenation with hashes with integer values.

I'm willing to bet that the latter is _way_ faster due to the number of operations your CPU has to do with comparing numbers versus strings. Since JavaScript uses reference pointers internally for pretty much anything, I would bet that this _will_ make a difference, if only very slightly.

[oguzeroglu]

Another approach would be number packing if we want to go completely mental in this issue :) For instance if both stateID, directionalLength, pointLength etc. had limits (lets say 5 bits per variable) we could pack them into a 32 or 64 bits integer and obtain them individually using bitwise operators, but it's probably more sane not to deal with that.

[Usnul]

Well, to avoid guesses as to my position. I believe a 32bit integer is sufficient in a lot of cases, and beyond that I would guess a decent 128 bit non-cryptographic hashing like metro-hash is a good solution.

A note about 32bits - integers in JS are 32 bit explicitly, whenever you decide to use any bit twiddling operations - a number gets truncated to a 32bit int.