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

Implement get_node with a get_node_raw #14384

Merged
merged 4 commits into from Mar 3, 2024

Conversation

Desour
Copy link
Member

@Desour Desour commented Feb 18, 2024

  • Goal: Improve performance of get_node.
  • How: get_node is implemented in builtin with a get_node_raw(x, y, z) -> content, param1, param2, pos_ok.
  • Why is this beneficial: In master, get_node calls lua functions to push nodes, and to read vectors. This is much faster if done in lua.

Flamegraphs:
master:
master
PR:
pr

Rough benchmark results:
master: 450-850 ms
PR: around 270-300 ms (it's much more persistent, idk why)

Benchmark function adopted from #14225 by @sfence. (Added you as co-author. :))

To do

This PR is a Ready for Review.

How to test

  • /bench_bulk_get_node
  • TODO: Do we have unit tests for get_node and get_node_or_nil?

Here's some instructions I've noted down at some point to disable cpu scaling, for better testing:

info:
	https://www.kernel.org/doc/html/v5.11/admin-guide/pm/cpufreq.html
	https://wiki.archlinux.org/index.php/CPU_frequency_scaling


check:
	sudo cpupower frequency-info
	watch grep \"cpu MHz\" /proc/cpuinfo


cpu-scaling off:
	sudo sh -c "echo 0 > /sys/devices/system/cpu/cpufreq/boost"
	sudo cpupower frequency-set -g userspace
	sudo cpupower frequency-set -f 1600MHz

cpu-scaling on:
	sudo cpupower frequency-set -g schedutil
	sudo sh -c "echo 1 > /sys/devices/system/cpu/cpufreq/boost"

Co-authored-by: SFENCE <sfence.software@gmail.com>
@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 21, 2024
@Desour
Copy link
Member Author

Desour commented Feb 23, 2024

Added the comments.

@Desour Desour removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 23, 2024
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise

builtin/common/item_s.lua Outdated Show resolved Hide resolved
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  Time / ms PR / ms
  314.16 79.64
  290.42 81.25
  254.11 80.21
  193.72 80.7
  184.71 80.32
  329.17 80.05
  251.46 80.42
  301.68 79.2
  197.04 79.23
  197.5 80.82
     
average 251.4 80.2
MEDIAN 252.8 80.3

I find it strange that there's this much difference. pushnode already uses Lua functions to construct the table from arguments (core.set_push_node). So apparently by moving the table constructor after "returning" from C++ makes such big difference? Why's that?

@Desour
Copy link
Member Author

Desour commented Mar 2, 2024

In master we call lua functions from C++ to read vector and push node values. The C->Lua call overhead is probably quite big. And also we always need to look up the functions to call first, which requires a bunch of Lua C api calls.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well. Cannot complain.

@SmallJoker SmallJoker merged commit d4d4712 into minetest:master Mar 3, 2024
15 checks passed
@Desour Desour deleted the get_node_pushnodeinlua branch March 3, 2024 15:09
@Desour
Copy link
Member Author

Desour commented Mar 3, 2024

Benchmark function adopted from #14225 by @sfence. (Added you as co-author. :))

Somehow that co-authorship didn't make it into the commit. Did github squash it away?

@SmallJoker
Copy link
Member

SmallJoker commented Mar 3, 2024

Sorry @Desour (or rather @sfence ). I totally missed that remark and removed it alongside the other commit titles upon merge. GitHub is not to blame.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants