Closed SafeteeWoW closed 3 years ago
Agreed. One of my goals is to first and foremost eliminate the variable names, as they currently makes up a lot of the data. A function could easily build and reconstruct it whilst keeping track of data added by modules. I even considered writing a library for handle that stuff.
I don't necessarily agree with more comunication = more bugs. In fact in v1 item data wasn't sent, and people had to gather that data themselves. It caused a lot of bugs getting everything to sync properly not to mention the slowdown involved in it (just think about how long it sometimes takes when doing /test). Also there's only backwards issues when stuff is changed.
I still think there's more pros than cons for not having clients rely on gathering the data for themselves.
While some stuff like awarded and quality isn't needed (and could probably be removed now even) it still only takes up a few bytes (the table name aside), and stuff like equiploc enables you to send a candidates gear right away, and not having to wait ~3 seconds for that.
A few more to the list:
addon.UI
and addon.Util
can really cleanup some confusion. Sync and to an extend lootframe are examples of this.I may slowly write some code for RCLootCouncil3.
I want to remove autoAward feature. I don't understand why any guild would use it. I never see any non-Epic items that can be master looted in the raid. Does those kind of items exist in the earlier expansion? And I dont think anyone autoAward epic items
See this for performance optimization: https://www.curseforge.com/wow/addons/findglobals
Recent bug reports about disconnect suggest more that we shouldn't send unneeded data. Usually server only disconnects you when you send too much data. It should not disconnect when addon has LUA error. If stuck in infinite loop, the client stop responding, but no disconnection
Yeah, but when are we sending/asking for too much (potentially repeating) data? This can't possibly be about the data sent between clients, it has to be about processing data locally - the excact thing I've been trying to avoid. This has never been an issue before!
You'll crash if you break lua rules (most likely infinite loop), you'll get disconnected if you break Blizzard stuff. I'm still not seeing any reasons from the current issues that should cause disconects (except for the 30 people count).
I haven't really seen your edited comment, and not considered your autoAward
comment (just for clarification).
For the find globals stuff - I have begun that, but that has been down prioritied during your recent additions. But yeah, it's probably one of the biggest performance enchancements that can currently be done (LibDebug stfu much?).
Did people report that the game freezes? If not, disconnection is not caused by infinite loop. Long ago, EPGP module has a bug causes infinite loop. It freezes the game, but no disconnection.
Auto award was a very requested feature back in the day for giving out random greens/blues to disenchanters.
GetItemInfo()
instead of 6. It would require you to potentially make a lot of delays to ensure you've received that data 30 times. Also when considering the current DC problems, this is a way to lower output.GetItemInfo()
instead of just 66. Even if it would take extra time, we're still talking milliseconds, and then you haven't counted the extra server requests needed. Don't start with data losses - they might aswell occur in our server requests. Besides, in this case number of requests is generally more important than the size of them due to fixed sized packets.GetItemInfo()
call from clients. Now, I don't know exactly how that's sent, but assuming it only sends data not available in GetItemInfoInstant
and it's somewhat clever with not sending the name twice (name included in link), with AceSerialize that would still amount to 143 bytes. Let's assume Blizzard uses something way more efficient than that, so lets say it's only 1/4 of that (~36 bytes). That's another 66 * 36 = 2,376 kB, meaning the gain is only 2,706 kB overall (29%) (This difference even almost goes away if the efficiency is only 50%). This nicely fits within the 4 kB burst ChatThrottleLib allows.
Of course this number would go up and down depending on the item, but I think it's safe to say there's benefits in only doing the caching once.GetItemInfo
might also be lost. We shouldn't really think about that, as mentioned above.Now, you could argue all clients will do a GetItemInfo
anyway to actually see the item (which is true), but it's still not that much data needed to ensure everything works instantly.
TLDR, GetItemInfo does not query the server. It fetches information on the disk to the memory. Since GetItemInfo does not involves network traffic, it should both faster and more reliable than transmitting item info over addon channel.
It seems raid no longer drops green/blue stuffs. I assume AutoAward feature can be removed later.
Dont equalize 6 calls on 30 machines each with 6 x 30 calls on single machine. If things run in parallel, we should consider the performance as 6 calls instead of 180.
Item link can be compressed even further. At least Color code, item name can be removed from the link. |Hitem:151982::::::::110:264:::::||h
. This is only 39 bytes. Colons in the item link can also be compressed in some way. Btw, if the item is already cached, calling it with item name and color code removed does not cause it to recache. If I tranmit the item link with some ways to compress the repeated colons, I can reduce the size further. Ultimately, it's may be possible to let 255 characters to contain 6 items.
GetItemInfo doesn't always send network request if it is not cached in memory. It first looks at the disk cache. The example is this code https://gist.github.com/SafeteeWoW/ccfa4e6f3fef81d36f9e69b9ed5fb7bb In one experiment, I got GET_ITEM_INFO_RECEIVED on the next frame(after 0.01s) of my GetItemInfo(). If this is sent by network request, this cant be this fast because I have 60 ms latency. This is why GetItemInfo is fast most of the time. Some item id even gets GET_ITEM_INFO_RECEIVED on the same frame.
You are assuming the raiders always need to fetch the item info when the lootTable is received, however, it is not true. If the item data is already cached. Then ML sends the item info is completely not necessary. Here are my observations.
It never takes more than 0.2s for me to cache all Antorus items. No matter if I delete the WoW cache folder, restart the game client, etc. We see items in '/rc fulltest' need more time to be cached because we only refresh it every 1s. It's a performance lost if you transmit all item info. The additional information takes at least an extra second to transmit instead of 0.2s.
Suppose GetItemInfo actually does not query the server, I think fetch information on the disk is always more reliable than sending anything on the network. Then GetItemInfo(data from disk) should be always both faster and more reliable than transmit data over addon channel. Besides, it also makes coding easier.
Again tested on PTR, but I have reinstalled the PTR game client. The only addon enabled is WoWLua. The code is https://gist.github.com/SafeteeWoW/7a4a63770870af06717830d871619242 The the majority of GetItemInfo, the info is fetch in the next frame (9-10ms for me@100FPS). This should be enough to confirm that GetItemInfo does not query the server. There are some occasionally several hundreds ms of GetItemInfo, but that's the natural of disk. Although somehow my latency drops to 30ms somehow, the network still won't allow me to receive server message within 10ms.
Then I did an ultimate test. While running the above code, I unplugged my computer's network cable. But the above code still runs. After 5s, WoW finally decides to tell me I'm disconnected.
Another issue is that why I cannot cache all items using for i=1,200000 do GetItemInfo(i) end
. I think there is an upper limit (~32k) how many items can be cached inside the memory. Thus the above code never cache all items.
Why are there some long retrieving item infos on login? There are usually lots of item needs to be cached on login, especially if you login to a major city. Caching hundreds of items together takes some time. Beside, the disk is busying reading some other stuffs at that moment. This can also explain why retrieving item info is rarely seen during the raid.
Let's talk about math. Currently an item takes 289 bytes, so 6 items -=1734 bytes ~= 7 messages (6.8*255). Compressed item links 43 bytes, 6 items = 2 message, considering some additional data. Overal (7-2)/7 = 70% less data.
In the current RCLootCouncil version, it seems 1s to too much for recaching the item. I suggest to change it to 0.2s
It seems you're right.
GetItemInfo
did receive data from the server, then it would be fair to consider the total implications.{link = "item:151982::::::::110:265::5:3:3611:1487:3528:::"}
. I should have removed "link", although there's only a 3 byte difference as it'll get number indexed. Also "|H" and "|h" is not part of the itemstring, only the itemlink.
Yes, GetItemInfo
takes itemID or "itemString" or "itemName" or "itemLink" as arguments. You can also remove trailing colons, as it's still a valid itemstring. Removing colons from the middle of the string is probably not worth it.GetItemInfo
would be received the next frame.GetItemInfo
does not query the server.Furthermore I found that:
GetItemInfo
returns nil) is really a "not in memory" item and not "not on our disk" (I know that's basically the definition of cache, but you get the point).GetItemInfo
calls. This explains why you had some items that never showed up.Also, Wowprogramming really isn't reliable for anything that's been changed in the last two expansions (quite good for old UI/event stuff though). The wikis are generally much more reliable.
local libS = LibStub:GetLibrary("AceSerializer-3.0")
local libC = LibStub:GetLibrary("LibCompress")
local libCE = libC:GetAddonEncodeTable()
local t = {}
for k, v in pairs(RCLootCouncilML.lootTable) do
local link = string.match(v.link, ".*|Hitem:(.-):*|h.*")
t[k] = {['\128'] = link, ['\129'] = true}
end
local s = libS:Serialize(t)
print(s:len())
local c = libC:Compress(s)
local f = libCE:Encode(c)
print(f:len())
So I run '/rc fulltest 6" then run this code. It shows the lootTable now only takes 163 bytes to transmit. I have an idea to compress data. We can have a dictionary to replace commonly used word, if they are used as the key to the table. For example, we can reserve all ascii value >= 128 and replace "link" by "\128", replace "bagged" by "\129", etc. Those stuffs do takes lots of spaces. Because the addon is communication heavy, I do want to make the messages in most cases be within 255 character limit.
Further more, duplicate items will be shown in the same session. Just need to adjust the UI a little bit.
I don't think it's worth it to do string replacements. If we know lootTable will always only contain link and bagged, then a number indexed table will do just fine.
I can't imagine a good way to display several equal items in the same session - it will be confusing, and imo showing two tabs is better. It might be worth combining equal items when sending out the lootTable though - that's easily done both when sending and recreating at the receiving end.
Also, what's the result print(s:len())
in your example?
In another "/rc fulltest 6". Before compress = 286 -> 2 messages. After compress = 175 -> 1 message. This is big improvement
Example:
item:151982::::::::110:265::5:3:3611:1487:3528 -- trailing colon not needed
item:151982:::::::::265::5:3:3611:1487:3528 -- player level does nothing unless the item is heirloom.
item:\015\019\082:::::::::\002\065::\005:\003:\036\011:\014\087:\035\028 -- We can change the base of number to compress it further. For example, use base 100 and use one ASCII character to store on digit. (\015 is one ASCII character with the value 15) (This is just an example. The real one will avoid to use ASCII characters that are escaped by AceSerializer)
item:\015\019\082&9\002\065::\005:\003:\036\011:\014\087:\035\028 -- Repeated colons can be reduced by some encoding.
it:\015\019\082&9\002\065::\005:\003:\036\011:\014\087:\035\028 -- Change the prefix "item:" to sth shorter but unique.
Before compress: 48 bytes
After compress: 24 bytes
I only asked because I've always found encoding to increase the size.
There's a huge difference in doing replacements in table names and actual data. The latter is fine, provided it isn't slow.
I did a quick test of your encoding:
Data example (not the string used in the first example)
Method 1 : "item:137487:5890::::::::::35:3:3418:1597:3337"
Method 2: "it:\013\074\087:\058\090\082&9\035:\003:\034\018:\015\097:\033\037"
. | Raw | Serialized | CompressedLZW | Serialized + Compressed |
---|---|---|---|---|
Method1: | 43 | 49 | 44 | 50 |
Method2: | 24 | 38 | 25 | 39 |
Edit: 4 items doesn't really make sense.
. | Serialized | CompressedLZW | Huffman | Huffman + Encode |
---|---|---|---|---|
Method1: | 326 | 327 | 192 | 205 |
Method2: | 243 | 244 | 233 | 236 |
~I only used LZW compression,~ as it's the only one I exported to run out of the game.
Also, you don't need to transmit "item" - it can always be added on receive as we know it's needed.
So is it worth it? Maybe, not sure.
In the previous example, once any other data is added, it would be two messages any way, and so far my testing shows a message takes equal time no matter how big that message is (as it really should).
I expect most transmission to be shorted down from 2 to 1 with this change, but it really depends on what needs to be sent.
Goal of communcation handling in RCv3
Small commands sent together should be combined into one big command. Send 2 messages with 250 characters should be much faster than sending 10 messages with 50 characters.
When session contains 6 session in 30man raid, any command from non-ML should not exceed 255 characters.
It's very possible that MLdb is within 255 characters if key are number indexed (I don't like this) or there is some string replacement.
candidates improvement.
name
, class
and role
should be transmitted through addon comm. There is no need. The game client always fetch these information anyway and these information can be fetched by API. It just redundant to transmit these information again when the game client has done it. While you can say it's possible that the candidate list may be different with ML if it is not transmitted, but it will only happen on logged in when GetRaidRosterInfo is not ready. We can simply delay any other operations until GetRaidRosterInfo is ready.council improvement.
57-0A0BF579
, this is 11 characters per candidate. If we also consider the serializer cost ^S57-0A0BF579
, that is 13 characters per candidate.
lootTable improvement.
General improvment
local toSend = self:Serialize(command, {...})
should be changed to local toSend = self:Serialize(command, ...)
AceSerializer.
I only asked because I've always found encoding to increase the size.
The main reason is that it is an example and the encoding didn't avoid the characters escaped by the serializer. non-print characters such as \003 is prefixed by an additional escape character. And the real one should be base 220 instead of base 100.
Can you test again, but replaced all ASCII characters by adding 32? i.e. replace \003 by \035, \002 by \034 I'll do a test myself.
Yep, I can confirm it's not worth it. after tested myself.
Yes, Huffman compression is more effective than the version of your encoding I used. I'll update my result for clarity.
As for your communication goals:
Agreed, however it probably doesn't happen that often.
Also agreed, but I'm not sure it can be guaranteed. Take the current lootAck
as an example.
Space wise, it would be the same (presuming you'd use 1 byte keys). Depending on the serialization method there could be some (minor) gains to both solutions. I agree mldb
is a good example of where to use string replacements instead numbers, as guaranteeing the order could cost extra data if it doesn't need to be sent. Elsewhere (e.g. lootTable) there's no real benefit in using strings.
I agree. And since GUIDs are permanent it's easy to store a GUID to playername/class table in SV.
Always assume worst case.
I agree GUIDs should be used here as well (actually anywhere where realname is currently used).
Doing Serialize(unpack(council))
is not worth it. In general cases (say 6 councilmen) you'll only save 22 bytes. With many more (18 in my case) it grows to 67 bytes. Contructing the council like [name] = true
makes the previous mentioned 18 strong council go from 461 bytes to 369, 25 better than unpacking first. It has the added benefit of making looking up values faster.
and 7. I'm not really sure it's worth it. For lootTable you'd at most save 22 bytes (which presumeably shrinks to almost nothing once compressed). Not to mention you'd also have to handle the unknown number of arguments on receiving (although it's easy to make a function for that).
For debugging it doesn't really matter how it's serialized/encoded, as long as it's consistant. It's easy to write a decode out of game. I'm not sure if it's really worth it in the end, but feel free to look into it.
One thing to remember when talking encoding/serializing/compression - it can't have too much overhead. If each and every command sent needs a different encoding just to squeeze out a few more bytes, then it's probably not worth it.
Next I want to talk about the code structure. I think a very good example is the addon TradeskillMaster. That is a large project with well designed structure.
I generally agree, and already mentioned that earlier. I don't think it's inherently necessary to separate every widget into their own file - TSM does it as it is creating AceGUI widgets, which generally are bigger than those used in RCLootCouncil.
Can you include test script in the repository? We can exclude it in .pkgmeta And I found many good test script examples in Ace3 repo; https://repos.wowace.com/wow/ace3/trunk
Started to use this for testing: https://github.com/IUdalov/u-test
I have done a new serializer (Some extra tests need to be added later), which roughly 20% smaller after Compressed by LibCompress compared to the compressed result of AceSerializer. (Tested on RCLootCouncil.db.profile and RCLootCouncil.mldb("/rc fulltest 6"). Feel free to give suggestion how to optimize it. (For smaller data that can be contained within 255 characters, roughly 5% smaller)
Optimizations of this Serializer:
Cons:
Code to compare RCSerializer and AceSerializer: https://gist.github.com/SafeteeWoW/73fcc52a141376c200acab0606555c75 My Saved Variable for reference: https://gist.github.com/SafeteeWoW/3f48ea63035246353fccc39467a9ea92
RCSerializer-3.0: https://github.com/SafeteeWoW/RCLootCouncil3/blob/master/RCLibs/Serializer.lua Test Suite: https://github.com/SafeteeWoW/RCLootCouncil3/blob/master/test/RCLibs/Serializer.lua
I want to release this serializer as a standalone version, but I have to get rid of all Ace3 code in order to follow its license.
30% smaller than AceSerializer after LibCompressed for this data: https://gist.github.com/evil-morfar/b2706d1954c623b292b890d9d04fc19c Correctness has been verified.
Folder for tests/scripts along with those I've been using so far: 0f916425abaf064fdc156d983c944d7cae649750
Keep in mind I did not intend anyone but me to see those, hence the mess. It was originally created using extracts of code (like Serializer) as I wasn't aware someone had made a wow_api.lua
.
Great job with the serializer. Are you certain the reserved bits can't be in the text? /009 is tab, which might be included. Of course it could be a requirement not to use those, but I'm more curious about the others.
Also IntToCompressedInt
and the like; does it really belong in the serializer?
Do you have an idea of how much more cpu time this costs?
The serializer intends to work with any data except function/userdata/thread/NaN(Not a number). \009 is supported by the serilaizer. It is escaped in the serializer as \001\056 The escape character \001 is escaped as \001\048
If the string was seen more than once during the serialization, it is encoded as a number index instead. IntToCompressedInt
is used so I can encode two digits numbers in one byte. This operation does save space after Huffman compared by encode the index in decimal.
For the CPU part, I tested with your extreme data in game, https://gist.github.com/evil-morfar/b2706d1954c623b292b890d9d04fc19c, timed by debugprofilestop()
AceSer: 4.24ms (67537 bytes) RCSer : 22.82ms (34502 bytes)
However, because RCSer generates smaller data than AceSer, less CPU used when compressed by LibCompress.
Compress AceSer: 78.27ms Compress RCSer: 42.69ms
Overall, RCSer uses less CPU than AceSer if compressed by LibCompress
AceSer+Compress: 93.51ms (33918 bytes) RCSer+Compress: 72.9ms (22522 bytes)
Then I tested with my MLdb(RCLootCouncil.mldb):
AceSer: 0.38ms (2042 bytes) RCSer: 1.69ms (1116 bytes) Compress AceSer: 3.72ms Compress RCSer: 2.53ms AceSer+Compress: 3.98ms (1519 bytes) RCSer+Compress: 4.17ms (1058 bytes)
RCSer has nearly the same performance as AceSer
In conclusion, although RCSerializer is slower than AceSerializer, however, since it generates smaller data, its performance impact is canceled by the less CPU usage when compressed by Huffman encoding. And I haven't considered the fact slightly more CPU needed to transmit larger data. Thus, no significant performance difference compared to AceSerializer.
Due to my limited time, the ML changes, BfA release, and presumably patch 8.0 a month before, here's my current plans for future updates:
2.7.10 Practically ready. Contains the current master branch + a few smaller fixes. Release: some time next week.
2.8 This will contain a fully working PL mode using the current framework, just to get it working and tested before BfA. This should also include a new trade UI for raiders, along with any finished improvements in the backlog (I probably won't be able to spend too much time testing/fixing stuff that's not ready). Release: ASAP.
2.9 / 3.0 (patch 8.0 - probably july 3/4) This would be the optimal time to have a new comms system running (which includes a potential new serializer). It'll be 3.0 if the comms are ready, otherwise 2.9 with 8.0 API/toc updates, along with anything else finished. I'll start work on comms as soon as 2.8 is done.
3.0 / 3.1 (BfA launch, august 14) Last chance to do comms update (the absolute latest will be when the raid opens, presumably a week later). If comms are finished before this, then I'll work on any unfinished, planned features for this update.
3.1+
Here's when code structural changes can happen, i.e. most of the things discussed in this thread + #165. Somethings might make sense to change earlier (depending on where it makes sense with other changes - stuff like lootTable
, candidates
and council
needs to be updated with comms, while UI can probably wait). I'll make sure to create anything new (e.g. trade UI) with the discussed changes in mind.
That's a good plan. I would help if I can. But my current priority is the new Compressor (already released, hope you can help to test it) and the new serialize (50% ready. working on it this weekend). Getting communication quicker will resolve lots of issue (Also, we don't need to transmit item info. I dont have time to update my PR to the item info module though, but it should mostly works). As I tested out, using the new compressor and my WIP serializer, transmitting all item equipped on a player (removing "item" and trailing :::: needs no more than 2 WoW messages (< 500 bytes), so consider if you would do that.
I'll make sure to test it during development. Also just fyi, I have beta access (and alpha) if you need something tested. I'll probably develop 3.0 on the beta.
As usual things are delayed (this time mostly due to the reduced raiding in my guild meaning me being unable to test stuff when I had a lot of time for it). This means PL isn't quite ready, but will be released soon with patch 8.0 updates as v2.8.
This will need a few updates, which have first priority, but after that I'll focus on v3.0 (skipping 2.9) as per my post above. Any updates on the compressor and/or serializer?
Hi, I was afk this month, spending the nights on FIFA world cups. So there is no progress this month. However, the Compressor is 100% complete already. The new serializer currently makes it 20% more smaller, but I am not satisfied yet. I have restarted the searializer project today and hopfully give out a version Before August.
Yeah, world cups did take a lot of time :) I'm just finishing up the PL stuff, but right now it doesn't seem realistic with v3.0 for BfA release, as there's simply so much to do. I'll see how much I can get done during the next week or so now that I believe 2.8 is done.
Please keep me updated on the progress on the serializer.
Just an update on v3.0. There's simply too many things that requires refactoring for it to make sense with the comms update. That combined with the PL issues (and the addition of Azerite Armor becoming tradeable) means I won't have v3.0 ready for BFA raids release.
For now I'm going to postpone it to the next major patch/raid release. This also makes it a lot easier to test changes, and allows me to focus on the current new features.
Although it would be a milestone for the next expansion, I just want to list the ideas for future reference.
Goal for RCLootCouncil3: