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

Dont use net.WriteTable for EGP #3141

Merged
merged 7 commits into from
Oct 28, 2024

Conversation

wrefgtzweve
Copy link
Contributor

@wrefgtzweve wrefgtzweve commented Oct 9, 2024

Massively saves on EGP networking,

Just as a simple example:
This took 113616 bits before
image

After the pr this took ~4840 bits

@wrefgtzweve wrefgtzweve marked this pull request as ready for review October 9, 2024 12:25
@wrefgtzweve
Copy link
Contributor Author

wrefgtzweve commented Oct 9, 2024

There's a few more usage of net.WriteTable across wiremod, i could individually fix them or i could make a wirelib Write/Read table, thoughts?

@thegrb93
Copy link
Contributor

thegrb93 commented Oct 9, 2024

Would prefer something that doesn't use compression. Compression and json together will very likely cause lag spiking.

I think json will also cause strings that are numbers to automatically be parsed into numbers.

@wrefgtzweve
Copy link
Contributor Author

The sfs library i linked would be perfect but it'd be quite large to add so im not sure if thats wanted

@Vurv78
Copy link
Member

Vurv78 commented Oct 10, 2024

@thegrb93
Copy link
Contributor

Vurv's suggestion is ideal but requires each EGP object's networking to be specified and reworked. You could also import SF's stringToTable and tableToString which uses StringStream (wire already has a copy of StringStream).

@wrefgtzweve
Copy link
Contributor Author

wrefgtzweve commented Oct 10, 2024

Vurv's suggestion is ideal but requires each EGP object's networking to be specified and reworked. You could also import SF's stringToTable and tableToString which uses StringStream (wire already has a copy of StringStream).

Should be doable, however using SF.TableToString im getting more than double the size than json (both compressed) while sfs is smaller and faster than both

image
image

@thegrb93
Copy link
Contributor

They use 32 bit floats for numbers instead of 64 bit.

@wrefgtzweve
Copy link
Contributor Author

Seems to go pretty much up to math.huge

@Srlion
Copy link

Srlion commented Oct 10, 2024

They use 32 bit floats for numbers instead of 64 bit.

No it does not lol, but still doesn't fit the max possible value in lua. I probably can work on fixing that.

local MAX_NUMBER = 1.7976931348623e+308
local MIN_NUMBER = -MAX_NUMBER

@thegrb93
Copy link
Contributor

thegrb93 commented Oct 10, 2024

Oh, I misinterpreted it last night. I only glanced at it and thought there was a condition to use 32bit ones. you ought to credit write_double btw @Srlion . Starfall wrote that

I'd prefer starfall's since stringstream is already in wire and sfs reimplements all of that, but do whatever works

@thegrb93
Copy link
Contributor

thegrb93 commented Oct 10, 2024

I'd guess most of sf's wasted data is coming from numeric array indices which could be easily reduced, but idk what your data is.

@Srlion
Copy link

Srlion commented Oct 10, 2024

Oh, I misinterpreted it last night. you ought to credit write_double btw @Srlion . Starfall wrote that

I don't think I took it from Starfall, there were lots of pack functions for double I was testing but haven't encountered Starfall's one. It was probably this
https://github.com/bakpakin/binser/blob/30e59591ce0e3c949312578ea67820bed10516c3/binser.lua#L102
or
https://framagit.org/fperrad/lua-MessagePack/-/blame/master/src/MessagePack.lua?ref_type=heads#L423
with some modifications from some other place but I can't really remember lol.

I'd prefer starfall's since stringstream is already in wire and sfs reimplements all of that, but do whatever works

If we gonna mention serialization then we can just say they all reimplement each other. sfs is inspired by messagepack, taking the part where it can handle small values really efficiently and adding the part where it does that really fast. If you don't like adding sfs, you could at least expose functions that can be detoured to allow using different encoders.

@wrefgtzweve
Copy link
Contributor Author

In the end all of them are better than the current net.WriteTable anyways lol
I should've probably had that in the comparison

@thegrb93
Copy link
Contributor

This should improve SF's, once I'm sure it's working. thegrb93/StarfallEx#1870

@wrefgtzweve
Copy link
Contributor Author

This should improve SF's, once I'm sure it's working. thegrb93/StarfallEx#1870

Old:
image

Using the new starfallex PR:
image

@wrefgtzweve
Copy link
Contributor Author

wrefgtzweve commented Oct 11, 2024

Turns out wiremod ships with von WireLib.von.serialize, might as well add it to the comparison
image

It's a little worse compared to json but it properly sends gmod types so i'll change this pr to use von instead, also doesnt require adding any extra libraries.

@Srlion
Copy link

Srlion commented Oct 11, 2024

Von shouldn't be used, don't take performance as the only measure, as it can't handle numbers correctly because it just uses tostring (by concating) so it shouldn't be included.

@Vurv78
Copy link
Member

Vurv78 commented Oct 11, 2024

If all you care about is encode time probably qjson would dominate the list. Although would need to be specialized to handle gmod types, probably.

But I don't think it should matter here anyway, this would rarely run. If JSON works properly, then the jump from WriteTable to it is already significant enough to take it for the low effort of swapping to JSON.

Then if you still want to network less, then use the typed networking library, or just manually network everything.

@Srlion
Copy link

Srlion commented Oct 11, 2024

If all you care about is encode time probably qjson would dominate the list. Although would need to be specialized to handle gmod types, probably.

Encode time it will perform the same as sfs, but decoding time sfs is unbeatable 😉

@Vurv78
Copy link
Member

Vurv78 commented Oct 11, 2024

If all you care about is encode time probably qjson would dominate the list. Although would need to be specialized to handle gmod types, probably.

Encode time it will perform the same as sfs, but decoding time sfs is unbeatable 😉

I mean, it's 10x larger and a binary format, so I sure hope it'd be, not testing tho

@thegrb93
Copy link
Contributor

@wrefgtzweve try once more now

@wrefgtzweve
Copy link
Contributor Author

wrefgtzweve commented Oct 12, 2024

@wrefgtzweve try once more now

egp test e2:

SFS Encode: 0.0011147000000165	16711	768
JSON Encode: 0.0020599000000061	27960	833
Starfall Encode: 0.0026024000000007	41358	940
Von Encode: 0.0023635999999954	27106	975

common acf hud e2 egp:

SFS Encode: 0.0006561000000147	3558	653
JSON Encode: 0.0007408999999825	6460	683
Starfall Encode: 0.0022637000000145	9943	796
Von Encode: 0.0010515999999825	5990	705

@wrefgtzweve
Copy link
Contributor Author

Here's a json of the data that egp sends if you wanna test around with SF.TableToString
egp_datastream.txt

@thegrb93
Copy link
Contributor

I think the only thing SF's is missing is encoding numbers as ints, which explains it since all your numbers in this are ints.

@wrefgtzweve
Copy link
Contributor Author

After testing with some other chips, JSON SFS and VON are all very close, sticking with VON seems the best option as it's already in wiremod and supports glua things like entities.

@thegrb93
Copy link
Contributor

I'm getting an encoded length of 12222 now with that data after my latest commit.

@thegrb93
Copy link
Contributor

After adding the int encoding.

@wrefgtzweve
Copy link
Contributor Author

acf hud:
VON: 7191 COMPRESSED: 973
JSON: 7861 COMPRESSED: 997
SF: 8020 COMPRESSED: 1087

benchmark egp:
VON: 25495 COMPRESSED: 1819
JSON: 28754 COMPRESSED: 1839
SF: 29097 COMPRESSED: 1882

Still roughly the same as von/json

@thegrb93
Copy link
Contributor

I'm satisfied with that. If you go with sfs, I'd prefer if it replaced the von lib that's currently in wire.

@wrefgtzweve
Copy link
Contributor Author

I was gonna do that initially but von can't really be replaced without breaking some e2 functions

local ok, ret = pcall(WireLib.von.serialize, data)

@thegrb93
Copy link
Contributor

Just change it to use sls?

@thegrb93
Copy link
Contributor

Would just need to warn people to convert any saved von files into sls format.

@thegrb93
Copy link
Contributor

Or we just use von and merge this. Maybe switch to sls later

@Vurv78
Copy link
Member

Vurv78 commented Oct 14, 2024

I would advise against merging in a library that is 1k+ lines of code for a marginal reduction in net bandwidth (compared to json/von), especially when it's supposed to be a temporary fix until the actual solution (typing the actual egp's structure) is done.

But I digress

@wrefgtzweve
Copy link
Contributor Author

I would advise against merging in a library that is 1k+ lines of code for a marginal reduction in net bandwidth (compared to json/von), especially when it's supposed to be a temporary fix until the actual solution (typing the actual egp's structure) is done.

But I digress

I think the real fix would be entirely ditching the SendDataStream logic and somehow replacing it with egps :Transmit :Receive as those are already typed properly, but yeah not fit for this simple fix pr

@Srlion
Copy link

Srlion commented Oct 15, 2024

Btw if you ever gonna use sfs, you can easily add custom types to fit your usage, here is how I add Console type in my admin mod:

local sfs = load_file("sam/libs/sh_sfs.lua")
sam.sfs = sfs

local Encoder = sfs.Encoder
local chars = sfs.chars
local write = Encoder.write

local CONSOLE
-- first argument is the type, eg. type(val) == "console"
-- the function returns what we can call an "id" to use when encoding
CONSOLE = sfs.add_encoder("console", function(buf, val) -- there are 31 free slots to use
    write(buf, chars[CONSOLE])
end)

sfs.add_decoder(CONSOLE, function(ctx)
    ctx[1] = ctx[1] + 1
    return sam.console
end)

-- you need to supply a custom type function so that when sfs checks types of values, it can
-- identify your custom ones
sfs.set_type_function(sam.type)

You can read how gmod types are implemented in sfs.lua. (they need some optimization but you will get the point)

@wrefgtzweve
Copy link
Contributor Author

The last commit should satisfy the last requested changes

@thegrb93 thegrb93 merged commit 0193910 into wiremod:master Oct 28, 2024
1 check failed
@wrefgtzweve wrefgtzweve deleted the improve-egp-networking branch October 28, 2024 21:11
@AshleyJamesy
Copy link
Contributor

This change has broken egps for players first loading into the server, they no longer get the "[EGP] 1 EGP Screens found on the server. Sending objects now...[EGP] Received EGP object reload. 1 screens' objects were reloaded"

It requires E2 to be reset for objects to be sent

@wrefgtzweve
Copy link
Contributor Author

If you link to a egp it does that sync automatically

@thegrb93
Copy link
Contributor

thegrb93 commented Oct 30, 2024

What do you mean? They are reporting that screens aren't syncing for newly joined players. Seems like an issue.

Comment on lines -646 to -658

local function initspawn(ply)
timer.Simple(10,function()
if (ply and ply:IsValid()) then
local bool, msg = EGP:SendDataStream( ply )
if (bool == true) then
ply:ChatPrint("[EGP] " .. tostring(msg) .. " EGP Screens found on the server. Sending objects now...")
end
end
end)
end

hook.Add("PlayerInitialSpawn","EGP_SpawnFunc",initspawn)
Copy link
Contributor

Choose a reason for hiding this comment

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

@wrefgtzweve Probably caused by removing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that should probably only run for screens, it was entirely useless for huds. When i tried the regular streams managed fine and didnt cause me issues but i guess it slipped through

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

Successfully merging this pull request may close these issues.

5 participants