remove seek from NetworkMessage

[xyron98]

Pull Request Prelude

• I have followed proper The Forgotten Server code styling.
• I have read and understood the contribution guidelines before making this PR.
• I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Remove seek functionality from NetworkMessage since it is not used anywhere in the codebase and can behave unexpectedly when used with addByte.

Issues addressed:

#4877

How to test:

Compile and run the server.

[ArturKnopik]

probably

forgottenserver/src/networkmessage.h

Line 125 in b227678

bool setBufferPosition(MsgSize_t pos)

also can be removed, needs to be checked

[MillhioreBT]

While it is true that the base does not use seek, you cannot be certain that someone else might use it. It is unsafe if used incorrectly, just like countless other things.

[ranisalt]

If seek is not used, tell is likely not used too

[Codinablack]

So the point is to just remove lua API from the end users because its not used in the datapack?

[MillhioreBT]

So the point is to just remove lua API from the end users is its not used in the datapack?

I thought that, in that case we must eliminate 50% of the LUA functions/methods that are not used in the Datapack
modalwindow, helper, event_helper, compat.lua, ect...

[ArturKnopik]

So the point is to just remove lua API from the end users because its not used in the datapack?

So the point is to just remove lua API from the end users is its not used in the datapack?

I thought that, in that case we must eliminate 50% of the LUA functions/methods that are not used in the Datapack modalwindow, helper, event_helper, compat.lua, ect...

please read issue linked in this PR, i hope you will understand why this PR exist

[MillhioreBT]

So the point is to just remove lua API from the end users because its not used in the datapack?

So the point is to just remove lua API from the end users is its not used in the datapack?

I thought that, in that case we must eliminate 50% of the LUA functions/methods that are not used in the Datapack modalwindow, helper, event_helper, compat.lua, ect...

please read issue linked in this PR, i hope you will understand why this PR exist

Wouldn't it be better to add a check? Only increase if necessary.
I've been running some tests, and the time difference is negligible in Release mode.

Test Code

#include <benchmark/benchmark.h>
#include <cstring>
#include <type_traits>
#include <vector>

struct BufferInfo
{
	std::vector<char> buffer;
	size_t position = 0;
	size_t length = 0;
};

template <typename T>
void add_with_check(BufferInfo& info, T value)
{
	static_assert(std::is_trivially_copyable_v<T>, "T must be trivially copyable");

	if (info.position + sizeof(T) > info.buffer.size()) {
		return;
	}

	if (info.position + sizeof(T) > info.length) {
		info.length = info.position + sizeof(T);
	}

	std::memcpy(info.buffer.data() + info.position, &value, sizeof(T));
	info.position += sizeof(T);
}

template <typename T>
void add_without_check(BufferInfo& info, T value)
{
	static_assert(std::is_trivially_copyable_v<T>, "T must be trivially copyable");

	if (info.position + sizeof(T) > info.buffer.size()) {
		return;
	}

	std::memcpy(info.buffer.data() + info.position, &value, sizeof(T));
	info.position += sizeof(T);
	info.length += sizeof(T);
}

static void BM_AddWithCheck(benchmark::State& state)
{
	BufferInfo info;
	info.buffer.resize(1024 * 1024);
	int value = 42;

	for (auto _ : state) {
		add_with_check(info, value);
		info.position = 0;
	}
}
BENCHMARK(BM_AddWithCheck);

static void BM_AddWithoutCheck(benchmark::State& state)
{
	BufferInfo info;
	info.buffer.resize(1024 * 1024);
	int value = 42;

	for (auto _ : state) {
		add_without_check(info, value);
		info.position = 0;
	}
}
BENCHMARK(BM_AddWithoutCheck);

static void BM_AddWithCheckOverwrite(benchmark::State& state)
{
	BufferInfo info;
	info.buffer.resize(1024 * 1024);
	info.length = 512 * 1024;
	int value = 42;

	for (auto _ : state) {
		add_with_check(info, value);
		info.position = 0;
	}
}
BENCHMARK(BM_AddWithCheckOverwrite);

static void BM_AddWithoutCheckOverwrite(benchmark::State& state)
{
	BufferInfo info;
	info.buffer.resize(1024 * 1024);
	info.length = 512 * 1024;
	int value = 42;

	for (auto _ : state) {
		add_without_check(info, value);
		info.position = 0;
	}
}
BENCHMARK(BM_AddWithoutCheckOverwrite);

BENCHMARK_MAIN();

Honestly, scenarios where add<T> is called hundreds of thousands of times consecutively seem like something that never happens. And if they did occur, the overhead would be a couple of nanoseconds.

[ArturKnopik]

the increase is at 30-35% and this code is commonly used almost everywhere, I prefer not to add checks, it's too big increase in execution time...
everything that happens almost always triggers sending some networkmessages:

• movement (player rotation, sending of map + possibly a monster if present),
• magic effect,
• distance effect,
• messages to player/channels
• market
• from latest protocol preys/bestiary/boostiary (timeout is updated by server)
• exp/mana spend/skills
• moving items
• etc...

[ranisalt]

This is a very hot code path and I'd rather not add the check. If not even the engine needs to arbitrarily navigate a network message, I don't think the end user needs to.

seek is not needed, tell is not needed either (it will always equal len), and NetworkMessage::setBufferPosition in C++ will be unused too.

[NRH-AA]

I vote we add: byteAppend(seekPos) stringAppend(seekPos) and they handle the seek, check and add the data, and go back to end. Features above all.

[alysonjacomin]

@NRH-AA I completely agree, the more resources the better, and your idea is great