HomeWildfire Games

A bit more cleanup in ProductionQueue.

Description

A bit more cleanup in ProductionQueue.

  • Removed some useless comments.
  • Don't assume an item only has either a tech or a unit in ProgressTimeout.
  • Don't initialise boolean values (refs. #5979).

Differential revision: D3739
Comment by: @wraitii

Event Timeline

lyv added a subscriber: lyv.Mar 25 2021, 5:11 PM
lyv added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/components/ProductionQueue.js
815

What was the rationale behind this?

Freagarach added inline comments.Mar 25 2021, 5:15 PM
/ps/trunk/binaries/data/mods/public/simulation/components/ProductionQueue.js
815

https://trac.wildfiregames.com/ticket/5979
If we don't need to keep serialising data that is unused for large amount of time, e.g. OOS logs become (a tad) more readable.

Are there things I am overlooking here?

lyv added inline comments.Mar 25 2021, 5:28 PM
/ps/trunk/binaries/data/mods/public/simulation/components/ProductionQueue.js
643

To not just complain, I like these changes.

815

Maybe explicitly defining serializable properties might be cleaner. And if performance is the concern, setting a boolean is definitely faster than deleting properties and re initializing them again. There is also the issue of type optimizations potentially.

Potential performance impacts would depend on the JS engine, but I recall from some earlier benchmark that delete was kinda expensive since further lockups would result in searching the prototype chain. Also, it might change the object layout. Moreover, SM JIT does not play well with delete on arrays and hitting array holes was like hitting a wall. Given the underlying JS array implementation, I would not be surprised if the same behavior was found in objects as well.

I haven't redone those tests in current SM version. So, take that however you will.

Freagarach added inline comments.Mar 25 2021, 5:42 PM
/ps/trunk/binaries/data/mods/public/simulation/components/ProductionQueue.js
815

Thanks for your insights!

I actually thought that delete-recreate was slower than resetting, but reasoned that in cases like this (blocked spawning doesn't happen often) it is better than keeping a variable for the entire length of the game.
That said, I did not think of lookup costs. I'll post your thoughts on the ticket as well.

wraitii added inline comments.Mar 25 2021, 5:51 PM
/ps/trunk/binaries/data/mods/public/simulation/components/ProductionQueue.js
815

I remember those points too, though perhaps not in such detail. IIRC delete played badly with the JIT in general, but we lack good tools to measure that currently anyways.

I'd agree in general that I think it's OK for 'rare' properties - I'd say here qualifies. Still, caution seems good, but I don't think this particularly needs changing.


IMO our lack of good tooling in current SM with regards to profiling this makes it somewhat moot. I doubt the real slowdowns are in these micro-optimisations very much.

lyv added inline comments.Mar 25 2021, 6:06 PM
/ps/trunk/binaries/data/mods/public/simulation/components/ProductionQueue.js
815

I don't think there are that much "really slow" functions in JS. Just a lot of "somewhat slow" adding up.

Most of these benchmarks were done back then for map generation code where this was found to be extremely true.

(I have no clue about JS simulation performance though and never checked either)