Add custom data batching

[Pirulax]

This is a separate thread of #638. I'd like to discuss custom data batching.
My thoughts are that it shouldn't be too hard, but we need to consider a few things:

• Is it actually worth it? Does it decrease CPU / Network usage for the client / server?
• The biggest overhead in setElementData is triggering the event system(takes 70% of the execution time)
• Can we could write the content of the big packet on a separate thread, after allocating the bitstream, and such? Is it thread safe? Is it worth it?
• What would be the default sync interval? 50 ms? Would we gather enough data to be worth it to make this change, in such a small time period?
• Could it possibly break existing scripts because of the delay?
• When do we merge the PR? It probably needs to be merged in 1.6, as we must remove the separate element data packets.

A few things to keep in mind when working on this issue:

• If the client(s) update the same element data that is in the send buffer, we must update the buffered element too.

[Pirulax]

My honest option is that with such a small interval of 50 ms, it would just add additional overhead(if a client updates the data we must update the one in the buffer).

But then, there are time when we load a big batch of data(loading vehicles from custom vehicle manager systems, accounts, maps, etc..), but i dont think that it would compensate that much.

Perhaps add a new argument to setElementData to accept a batcher element, that is used to handle the batching, so scripters could manage batching more easily. Also if we would make it like that, we wouldn't break backwards compatibility. If the client doesn't support batch element data we would just sync it to them one by one. Later, in 1.6 remove the one-by-one thing.

alternative to the batcher element would be just a string specifying an internal queue name(like in mysql), and we could add functions like sendElementDataQueue(string queueName), etc..

[Pirulax]

Qaisjp's suggestions:

// A passed in table would be sent in a batch
Ele:setData{
 key=value,
}

This is a very good idea IMHO. We could handle this as a batch.

local b = Ele:getDataBatch()
b:setData(...) -- x4
b:save()

This isnt bad, but we cant combine multiple element's batches together.

local b = getDataBatch()
b{element,
    key = value, -- x4
}
b{ANOTHERelement,
    key = value, -- x4
}

b:save()

I dont like this either, the reason being that the syntax is pretty bad.

The DC discussion can be found here

My new idea:

Maybe we could just send every data in a server frame? If theres incoming data change request then just add the changed data into the batch(change it if its already in it), and at the end of the server frame just sync it on another thread, if thats possible(and always wait for it to finish before a new server frame). We must retain the order of change(if theres an incoming packet we must move the existing batch entry to the end).

The whole element data queue should be handled in another thread, so performance there isnt an issue at all, since most of the time(99%) the main thread wont need to wait for the custom data queue thread.

We could use a combination of std::forward_list and std::map.
In the forward_list we'd keep the order(with pointers to objects), and in the map (or preferably CFastHashMap if it doesnt invalidate pointers to objects)(we dont have .emplace_back, so its crossed out) we'd keep the batch element itself.
The batch element should contain a copy of SCustomData(the copy should be made when the batch element is created, and it should contain the original value, and, before sending we can check against it, so we dont sync the same value), so can check against its current value.

Example CustomDataBatchItem data layout:

struct BatchItem
{
    // we can compare against this value when creating the packet: delete the batch item if its the same as the current value
    const CLuaArgument valueWhenInserted; 
    // must point to the current value, this value will be synced
    const CLuaArgument* const valueNow; // points to the CLuaArgument in SCustomData

    const CElement* const ownerElement; // or maybe ElementID
};

It should work just fine if we finish sending it before processing the packages(CGame::DoPulse -> g_pNetServer->DoPulse()).
Since the net server is async(afaik) sending data from another thread should be just fine.

A few things im not sure in:

• what if we have an event trigger packet, and there we set some data, and we trigger the client back. Example code:

triggerServerEvent("setCustomDatas", localPlayer);

addEvent("callback", true)
addEventHandler("callback", localPlayer,
    function()
        fuckKnowsWhat = localPlayer:getData("setdata");
        -- with my idea this probably wont work. 
        -- Perhaps we should send element data before every other packet, 
        -- so we make sure no such shit happens?
    end
);



addEvent("setCustomDatas", true)
addEventHandler("setCustomDatas", root,
    function()
        client:setData("setdata", 10);
        triggerClientEvent("callback", client);
    end
);

[sacr1ficez]

Although i'm far away from this element data madness (tables for the win), i'd like to share my thoughts on it.

Is it actually worth it? Does it decrease CPU / Network usage for the client / server?

Yes it is. I've had few chances to test it on PVP intensive server (where every damage for vehicle/player was consuming one network call). Reaching 40 players was enough to choke whole server. So i've implemented a buffer solution thanks to awesome person who taught me that - and that gave a server a new life. (greets IIYAMA)

alternative to the batcher element would be just a string specifying an internal queue name(like in mysql)

This is exactly the same idea i've realised with my data system based on tables.

My honest option is that with such a small interval of 50 ms, it would just add additional overhead(if a client updates the data we must update the one in the buffer).

If you are not sure, what to do in here, take a example from my data system which allows scripter to customise delay used in queue. Keep in mind that static delay won't fit everyone.

[Pirulax]

Well, the whole element data sync can just be moved to a separate thread. I'm really not familiar with the inner workings of the netcode / our sync logic tho, so I'm not sure whenever this would cause issues or not. The problem is that the packets could get out of order (that is, a setElementData might arrive later than expected). The solution to this would be to queue up everything sync related, and sync it at the end of the server sync frame.

This would just be a workaround that issue. The main issue with element data is the event it triggers (onElementDataChange), sadly theres not a lot we can do about it, since its implemented already.

[brzys]

What would be the default sync interval? 50 ms? Would we gather enough data to be worth it to make this change, in such a small time period?

I think it'll break a lot of resources, e. g scripts that creates vehicle, sets some data and then triggers client that gets this data immediately. It isn't proper design, but for sure people use timers in this case instead of some async confirmations

The biggest overhead in setElementData is triggering the event system(takes 70% of the execution time)

Is really onElementDataChange the main issue of poor performance in scripts that don't use setElementData properly? Of course, it is visible in benchmarks, but that's not the case in my opinion. I've seen some gamemodes with 100-200 players basing everything on element datas and performance browser always showed net server using 3/4 of CPU time instead of Lua VM. Too many packets increasing exponentially with the players count.
Focusing on optimising sync is a good way to improve performance for servers (that are not always properly coded)

[Pirulax]

Well, the event is the main problem client-side, while on the server, the issue is obviously the sync.
Existing scripts cant really be fixed.