Builtin/../falling.lua: Do not call nodeupdate() on node punch

[paramat]

Originally part of a pull request by tenplus1.
Calling nodeupdate() on node punch was added for 0.4.14 stable.
Each tool or hand punch was triggering nodeupdate_single() for neighbour
nodes and a possible chain-reaction of nodeupdates through connected
falling nodes.
Now only nodeupdate() on place or dig node.
/////////////////////////////////////////////////////

Originally part of tenplus1's PR #4620 this has been split off into a seperate PR due to controversy and the need for more opinions.
See #4620 for the arguments for and against.
To be clear i approve this change.
@sfan5 do you still approve?

[sfan5]

I'm neutral regarding this.

[Wayward1]

What about @nerzhul's offer here?
#4620 (comment)
Wouldn't this be a bigger improvement, and prove better than removing a gameplay element?
(I realize that won't lessen the calls though)

[paramat]

nerzhul - To optimize this we need a pure C++ SAO instead of a slowy LUA. If needed i have a working pure C++ implementation in my fork i can backport.

Worth considering, until then though we can use this PR.

[rubenwardy]

Is this noticeable at all?

[sofar]

I've been quite vocal about my disapproval of this change :)

The performance impact is entirely hypothetical, and it removes a fun and dangerous element from the game.

[paramat]

@rubenwardy concerning noticeable #4620 (comment)

[t4im]

You guys know though, that the primary instrumentation of the profiler are global callbacks like this very on_punchnode, right?

But no, profiling does not indicate an issue with it.
And i cannot imagine it being noticeable for a user under normal circumstances with that little overhead compared to other things, that should take precedence over non-issues.

Of course anyone disagreeing with the data is invited to review #4343 and the preceding PR for potential bugs, which i cannot rule out.

[paramat]

What about when punching occurs in a huge volume of sand? there's a chain reaction through connected sand nodes.
I don't care if a profiler shows no issue, it's still a useful optimisation that greatly reduces the amount of lua code run, and the original change was unnecessary.

[rubenwardy]

I'd like to see some profiler stats on this. Maybe with 20 players punching once a second in a desert.

[t4im]

Punching a flat desert on Vanilla 5.1, GC, Singleplayer, without actual falling, Leaf decay for scale.

instrumentation	calls	use %	avg µs	min µs	max µs
- on_punchnode[2]	1.1	1.3	159	75	557
- ABM: Leaf decay	600.9	1.7	42024	550	99040

Stats of a day from a busy server would obviously provide much better insight in what might really benefit from optimization during normal operations, even though it would mix punching of falling and not falling nodes.

[paramat]

Hmm that's surprisngly short, thanks. I won't get too concerned about this then and might close in a while if no extra support.

[rubenwardy]

How often were you punching? Once a second?

[t4im]

How often were you punching? Once a second?

I assume you ask to interpret the data.
But that's not call/second, that's mean non-idle call/step:

So on average there was 1.1 punches per server step, whenever there was any punch at all.
Punching happened in 1.3% of all sampled steps.
A single call took 75-557µs, with the mean at 159µs.

When leaf decay was in process, it run ~600 times on average, taking on average 42ms each time.
Only having a small profiling window, this isn't very representative of course. But it seems from that data, that a decaying leaf usually decays among friends.

[tenplus1]

We would really need information running server side with around 20 players running around punching all kinds of nodes to see it's improvement. Having the ability to disable this feature would be handy though.

[paramat]

Closing.