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.

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

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

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@paramat
Copy link
Member

commented Oct 14, 2016

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?

@paramat paramat force-pushed the paramat:punchupdate branch Oct 14, 2016

@sfan5

This comment has been minimized.

Copy link
Member

commented Oct 14, 2016

I'm neutral regarding this.

@Wayward1

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2016

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

This comment has been minimized.

Copy link
Member

commented Oct 17, 2016

Is this noticeable at all?

@sofar

This comment has been minimized.

Copy link
Member

commented Oct 17, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2016

@rubenwardy concerning noticeable #4620 (comment)

@t4im

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2016

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

This comment has been minimized.

Copy link
Member

commented Oct 18, 2016

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

@t4im

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2016

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.

Builtin/../falling.lua: Do not call nodeupdate() on node punch
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.

@paramat paramat force-pushed the paramat:punchupdate branch to 602aa33 Oct 18, 2016

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Oct 18, 2016

How often were you punching? Once a second?

@t4im

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2016

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 paramat closed this Oct 24, 2016

@paramat paramat deleted the paramat:punchupdate branch Oct 24, 2016

@paramat

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2016

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.