After measuring number of events like "COMBAT_LOG_EVENT_UNFILTERED" and "UNIT_AURA" fired in combat (for reference 50k-100k CLEU on Ragnaros 25 man, 18k UNIT_AURA for single Arathi battle) I decided to write a library that will work as alternative to regular SetScript/RegisterEvent way. I want it to accept code snippets (like, for example, SecureHeaders do) instead of functions, so library itself can mix snippets together, and install a single handler for every event, saving on lots of function calls (and possible WoW engine <-> Lua boundary traversals - I don't know exactly how heavy event handling system is).
Pros:
- Much less calls and event handling system work. In my modest raiding setup CLEU has 14 handlers, that means I'd be saving at very least 650k function calls and WoW engine overhead on Ragnaros fight mentioned above.
- Library give event arguments names for you. When argument order changes (like CLEU order did last two times) only this library needs to be updated and all dependent code will work just fine.
- While compiling final handler, library inlines for you some useful data like playerGUID or playerClass for guaranteed fast access.
Cons:
- Existing code must be reformated to snippets, though often you would just strip function header and end, add [[]] around it and change variable names.
- Though compiler tries to validate inbound snippets, it still not that hard to write faulty snippet and when one snippet fails, every snippet that was added after it won't get a chance to work (though it could be offset somewhat with good debug output or "safe" mode that reverts to compiling every snippet to separate function, allowing user to let all other snippets work at cost of decreased performance until faulty snippet is fixed or disabled).
Here's current version that I already experiment on, integrating it with local copies of RatingBuster, BuffEnough and Recount for example: LibEventSnippet-1.0.2.zip
(See LOE_test function at end of .lua for primitive test case.)
So, what do you think? Is this indeed a good resource save, worth migrating to? Is there any idea of a good name for such library? And, since it is a library, can somebody guide me to uploading this to WA in a way suitable for embedding?
accept code snippets instead of functions, so library itself can mix snippets together, and install a single handler for every event, saving on lots of function calls
This.
Regular way:
WoW
+handler function
+handler function
+handler function
Three calls, three passes over WoW-Lua border.
AceEvent:
WoW
+AceEvent handler function
+handler function
+handler function
+handler function
Four calls and overhead of AceEvent, just a single pass over WoW-Lua border.
New Library:
WoW
+ handler function loadstring()'ed together from all snippets.
Single call, no overhead at all, just a single pass over WoW-Lua border.
This idea is interesting, however :
1) according to your code, if any snippet causes an error, the whole handler is aborted and there is no way to know where the error comes from, is it ? (whereas AceEvent properly propagate errors and their stack traces),
2) moreover, as "return" is not allowed in snippets, code with "return short-circuit" should be rewritten or a function call should be used, which defeats the library purpose.
Not exactly. You get standard error output from compiled handler and by processing error text and matching line numbers against previously submitted snippets error handler can print accurate information about what snippet caused an error and which line it was. Such error handler is on my TODO list.
Indeed code using "return" will need to be modified for this library. You'll either have to correctly nest all "if"s or use local state variable that will instruct all further code in your snippet to pass.
Note, however, that purpose of this library is not to be universal event handler to replace AceEvent, but to provide zero-overhead handling for small number of events that are both high volume and often called in combat and thus are really time-critical. You shouldn't want to rewrite all your code to this format, unless it is simple enough to have no return.
Upon reviewing AceEvent I see that it actually costs not one, but two calls, one of them being slightly heavier OO call and CallbackHandler introduces lot of additional overhead too, so correct AceEvent scheme would be:
AceEvent:
WoW
+AceEvent handler function
+ OO call to CH
+ CH overhead
+handler function
+ OO call to CH
+ CH overhead
+handler function
+ OO call to CH
+ CH overhead
+handler function
Not exactly. You get standard error output from compiled handler and by processing error text and matching line numbers against previously submitted snippets error handler can print accurate information about what snippet caused an error and which line it was. Such error handler is on my TODO list.
And is much needed, how would an end user report such error to the right people when several addons (from different authors) use the library ?
I've just updated top post with link to new version. It introduces:
- Slight optimization for inserting ENV value.
- Visible errors for "return" in snippet text and compilation errors
- Standard Blizzard error handler hook that adds snippet line, event name and stack trace of snippet installation to handler errors (this can also be easily modified to work with alternative error handlers as well).
So, no good ideas for name? Shall I leave it as is then? :)
If anybody wants to try it out I can help with converting or even completely write patch for your addon myself. I'm very interested in spreading this lib as fast as possible for it to have maximum positive effect on performance. I'm tired of mid-battle hiccups. :P
Do you actually have proof that the event processing causes any performance issues? And that your thing solves it?
As far as i can tell, its mostly the things that are done with the data the event provides, instead of the overhead introduced by the event processing itself, which causes any performance problems, if at all.
Blizzard greatly improved the performance of the C->Lua context switch quite a while ago, so the usual RegisterEvent handler does not add any serious overhead anymore these days. It was an issue back in the day, which was the main reason AceEvent was really designed, to avoid the switches - but today, thats not critical anymore.
I seriously doubt that this overhead would actually cause problems, its more likely that the business logic in the addons is causing issues, if you're seeing any, and not the C->Lua context switch, or the one function call.
I also agree with what the previous posters said regarding error handling.
I looked over your code, and there doesn't seem to be anything to protect you against errors. If the first snippet in your chain throws an error, all event handlers will fail, and never execute. (I see you even admit that)
The only way to actually protect individual snippets would be using pcall, however that means you have to wrap every handler in a closure, making this equal to AceEvent, which is even slower then a RegisterEvent handler in your addon. IMHO, the possibility of my addon breaking just because some other addon has an error in its event handler is nothing i would be willing to accept as a drawback
So to conclude why i think this is a bad idea:
- Questionable if there really is a benefit, and if it outweights the added complexity
- No protection against errors in other addons
Performance issues can usually be much easier be resolved in the addon in question doing something wrong, rather then just the overhead from event handling.
There's not that many CLEU/UNIT_AURA addons and most of them are already underwent good amount of optimization - there's simply less to nothing left in business logic to optimize. But, as you can find out with a simple Google search, performance in several intense fights is still somewhat an issue.
Yes, I understand that savings are far from mind blowing and yes, I know about added complexity. I don't "admit" lack of error protection, I simply threw it away to waste less resources. Error reporting is done in last uploaded version and this lib was written with idea of any errors being quickly reported and fixed in mind. Most handlers for those events should be simple enough anyway.
When there's not much left to optimize, you can only squeeze those extra cycles from what remains.
Show some numbers supporting this performance gain you're advocating. Handwaving about how it "should" be better, won't convince anyone to try your method.
I've measured performance as I was tweaking Recount in last few weeks and average gain is 0.2 seconds for 50000 CLEU events per every registered CLEU handler in AceEvent, which in my opinion sums to quite a number in addon-heavy UI in raid fight. That's if you only count call overhead, since version handling for change in arguments of CLEU adds even more.
0.2 seconds, aka 0.000004 seconds per event, which means that even if 30 addons register CLEU, you gain 0.00012 per CLEU call, you barely manage to shave a millisecond off of execution times.
That's assuming your measurements are accurate as well. All I see is a library that adds needless complexity for an incredibly trivial amount of performance "gain".
The really interesting thing is getting info on how much CLEU / COMBAT_LOG messages actually cost these days.
The way I did it back in the day for OnUpdate was to register 10000+ empty handlers and statting GetTime() between the first and the last ones after verifying that OnUpdate handlers get called in the order they are registered.
For 30 handlers that'd be 6 seconds per 5 minutes of Ragnaros fight wasted doing something useless instead of drawing pretty images. Also, you DID notice that this measures only handler call overhead and such things as CLEU version difference processing for our Chinese friends can easily double time wasted?
... CLEU version difference processing for our Chinese friends can easily double time wasted?
No addon should be doing that kind of check every time the CLEU handler runs. There should be one check done at runtime (in the main chunk) and a different function assigned depending on the version of WoW running:
if GetBuildInfo() < X then
MyAddon:COMBAT_LOG_EVENT_UNFILTERED(...)
-- do stuff here for old clients
end
else
MyAddon:COMBAT_LOG_EVENT_UNFILTERED(...)
-- do stuff here for new client
end
end
I would like to add that registering 30 CLEU is improbable.
You literally shouldn't exceed 5-6ish on a full system (boss mod, threat lib, damage meter, combat text, plus 2 to spare).
That said I know that some addons register one CLEU per sub-module. But even that isn't that tricky, because even if you run 10ish CLEUs, as discussed the impact is still quite minimal.
To me it is simply not worth the risk of a maintenance nightmare to outsource registering to CLEU to a third entity, for that kind of a performance kick-back.
The real crunch to performance is down-stream. How much computation does an addon do per CLEU. This is where the optimization should be, and that, sadly, is the job of the individual addon's author.
Isn't mainland China also on Cataclysm now? Well at least on patch 4.2 I suppose ..
4.2.2, but 4.3 is (partially? I can't actually read Chinese, so about all I can tell is that it's downloading 4.3 patch files but not applying them yet) available on the background downloader, so it will probably be released soon.
I would like to add that registering 30 CLEU is improbable.
I will count mine.
1x BuffEnough
2x DMB (global and relevant module)
1x EavesDrop
3x MSBT (cooldowns, parser, triggers)
1x Omen (have CLEU version overhead)
1x oRA3
1x Recount (have CLEU version overhead)
1x LibInternalCooldowns
4x Quartz
15 handlers in somewhat modest setup, only counting addons available to general public. Some random user might as well install tons of "UI packs" and get much more - not everybody bothers to throw junk away.
If a user can't be bothered to spend a minute or two removing some of the 500 addons he installed in a fit of random clicking, I don't feel that we, as addon developers, should be bothered to spend any time or energy accommodating or even considering them.
As for your claim of "saving" 6 seconds over a 5 minute fight... for a user averaging 35 FPS in raid combat, even if all of the CPU time not spent on CLEU event processing was spent on frame rendering (which it never would, because the game client's CPU usage is not zero-sum), your proposal would add a meager 0.7 FPS. Even the most observant of users is physically incapable of noticing the difference between 35 FPS and 35.7 FPS, especially in the middle of a boss fight.
Asking people whose spare-time hobby programming is enjoyed by hundreds of thousands (if not millions) of users to spend time rewriting their addons to use your proposed library -- not to mention introduce a single point of failure by which one mistake in your code, or one day after a patch where you did not have time to update, would mean an enormous number of users unable to run their addons -- just to maybe add 0.7 FPS is simply not reasonable.
There's nothing wrong with wanting to optimize, but I think you're well beyond the threshold of diminishing returns here.
Rollback Post to RevisionRollBack
To post a comment, please login or register a new account.
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumAfter measuring number of events like "COMBAT_LOG_EVENT_UNFILTERED" and "UNIT_AURA" fired in combat (for reference 50k-100k CLEU on Ragnaros 25 man, 18k UNIT_AURA for single Arathi battle) I decided to write a library that will work as alternative to regular SetScript/RegisterEvent way. I want it to accept code snippets (like, for example, SecureHeaders do) instead of functions, so library itself can mix snippets together, and install a single handler for every event, saving on lots of function calls (and possible WoW engine <-> Lua boundary traversals - I don't know exactly how heavy event handling system is).
Pros:
- Much less calls and event handling system work. In my modest raiding setup CLEU has 14 handlers, that means I'd be saving at very least 650k function calls and WoW engine overhead on Ragnaros fight mentioned above.
- Library give event arguments names for you. When argument order changes (like CLEU order did last two times) only this library needs to be updated and all dependent code will work just fine.
- While compiling final handler, library inlines for you some useful data like playerGUID or playerClass for guaranteed fast access.
Cons:
- Existing code must be reformated to snippets, though often you would just strip function header and end, add [[]] around it and change variable names.
- Though compiler tries to validate inbound snippets, it still not that hard to write faulty snippet and when one snippet fails, every snippet that was added after it won't get a chance to work (though it could be offset somewhat with good debug output or "safe" mode that reverts to compiling every snippet to separate function, allowing user to let all other snippets work at cost of decreased performance until faulty snippet is fixed or disabled).
Here's current version that I already experiment on, integrating it with local copies of RatingBuster, BuffEnough and Recount for example:
LibEventSnippet-1.0.2.zip
(See LOE_test function at end of .lua for primitive test case.)
So, what do you think? Is this indeed a good resource save, worth migrating to? Is there any idea of a good name for such library? And, since it is a library, can somebody guide me to uploading this to WA in a way suitable for embedding?
-
View User Profile
-
View Posts
-
Send Message
Curse Premium-
View User Profile
-
View Posts
-
Send Message
Curse PremiumThis.
-
View User Profile
-
View Posts
-
Send Message
Curse Premium1) according to your code, if any snippet causes an error, the whole handler is aborted and there is no way to know where the error comes from, is it ? (whereas AceEvent properly propagate errors and their stack traces),
2) moreover, as "return" is not allowed in snippets, code with "return short-circuit" should be rewritten or a function call should be used, which defeats the library purpose.
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumIndeed code using "return" will need to be modified for this library. You'll either have to correctly nest all "if"s or use local state variable that will instruct all further code in your snippet to pass.
Note, however, that purpose of this library is not to be universal event handler to replace AceEvent, but to provide zero-overhead handling for small number of events that are both high volume and often called in combat and thus are really time-critical. You shouldn't want to rewrite all your code to this format, unless it is simple enough to have no return.
Upon reviewing AceEvent I see that it actually costs not one, but two calls, one of them being slightly heavier OO call and CallbackHandler introduces lot of additional overhead too, so correct AceEvent scheme would be:
AceEvent: WoW +AceEvent handler function + OO call to CH + CH overhead +handler function + OO call to CH + CH overhead +handler function + OO call to CH + CH overhead +handler function-
View User Profile
-
View Posts
-
Send Message
Curse PremiumAnd is much needed, how would an end user report such error to the right people when several addons (from different authors) use the library ?
-
View User Profile
-
View Posts
-
Send Message
Curse Premium- Slight optimization for inserting ENV value.
- Visible errors for "return" in snippet text and compilation errors
- Standard Blizzard error handler hook that adds snippet line, event name and stack trace of snippet installation to handler errors (this can also be easily modified to work with alternative error handlers as well).
So, no good ideas for name? Shall I leave it as is then? :)
If anybody wants to try it out I can help with converting or even completely write patch for your addon myself. I'm very interested in spreading this lib as fast as possible for it to have maximum positive effect on performance. I'm tired of mid-battle hiccups. :P
-
View User Profile
-
View Posts
-
Send Message
Forum ModeratorAs far as i can tell, its mostly the things that are done with the data the event provides, instead of the overhead introduced by the event processing itself, which causes any performance problems, if at all.
Blizzard greatly improved the performance of the C->Lua context switch quite a while ago, so the usual RegisterEvent handler does not add any serious overhead anymore these days. It was an issue back in the day, which was the main reason AceEvent was really designed, to avoid the switches - but today, thats not critical anymore.
I seriously doubt that this overhead would actually cause problems, its more likely that the business logic in the addons is causing issues, if you're seeing any, and not the C->Lua context switch, or the one function call.
I also agree with what the previous posters said regarding error handling.
I looked over your code, and there doesn't seem to be anything to protect you against errors. If the first snippet in your chain throws an error, all event handlers will fail, and never execute. (I see you even admit that)
The only way to actually protect individual snippets would be using pcall, however that means you have to wrap every handler in a closure, making this equal to AceEvent, which is even slower then a RegisterEvent handler in your addon. IMHO, the possibility of my addon breaking just because some other addon has an error in its event handler is nothing i would be willing to accept as a drawback
So to conclude why i think this is a bad idea:
- Questionable if there really is a benefit, and if it outweights the added complexity
- No protection against errors in other addons
Performance issues can usually be much easier be resolved in the addon in question doing something wrong, rather then just the overhead from event handling.
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumYes, I understand that savings are far from mind blowing and yes, I know about added complexity. I don't "admit" lack of error protection, I simply threw it away to waste less resources. Error reporting is done in last uploaded version and this lib was written with idea of any errors being quickly reported and fixed in mind. Most handlers for those events should be simple enough anyway.
When there's not much left to optimize, you can only squeeze those extra cycles from what remains.
-
View User Profile
-
View Posts
-
Send Message
Curse Premium-
View User Profile
-
View Posts
-
Send Message
Curse Premium-
View User Profile
-
View Posts
-
Send Message
Curse PremiumThat's assuming your measurements are accurate as well. All I see is a library that adds needless complexity for an incredibly trivial amount of performance "gain".
The way I did it back in the day for OnUpdate was to register 10000+ empty handlers and statting GetTime() between the first and the last ones after verifying that OnUpdate handlers get called in the order they are registered.
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumNo addon should be doing that kind of check every time the CLEU handler runs. There should be one check done at runtime (in the main chunk) and a different function assigned depending on the version of WoW running:
if GetBuildInfo() < X then MyAddon:COMBAT_LOG_EVENT_UNFILTERED(...) -- do stuff here for old clients end else MyAddon:COMBAT_LOG_EVENT_UNFILTERED(...) -- do stuff here for new client end endIsn't mainland China also on Cataclysm now? Well at least on patch 4.2 I suppose ..
You literally shouldn't exceed 5-6ish on a full system (boss mod, threat lib, damage meter, combat text, plus 2 to spare).
That said I know that some addons register one CLEU per sub-module. But even that isn't that tricky, because even if you run 10ish CLEUs, as discussed the impact is still quite minimal.
To me it is simply not worth the risk of a maintenance nightmare to outsource registering to CLEU to a third entity, for that kind of a performance kick-back.
The real crunch to performance is down-stream. How much computation does an addon do per CLEU. This is where the optimization should be, and that, sadly, is the job of the individual addon's author.
4.2.2, but 4.3 is (partially? I can't actually read Chinese, so about all I can tell is that it's downloading 4.3 patch files but not applying them yet) available on the background downloader, so it will probably be released soon.
-
View User Profile
-
View Posts
-
Send Message
Curse Premium1x BuffEnough
2x DMB (global and relevant module)
1x EavesDrop
3x MSBT (cooldowns, parser, triggers)
1x Omen (have CLEU version overhead)
1x oRA3
1x Recount (have CLEU version overhead)
1x LibInternalCooldowns
4x Quartz
15 handlers in somewhat modest setup, only counting addons available to general public. Some random user might as well install tons of "UI packs" and get much more - not everybody bothers to throw junk away.
As for your claim of "saving" 6 seconds over a 5 minute fight... for a user averaging 35 FPS in raid combat, even if all of the CPU time not spent on CLEU event processing was spent on frame rendering (which it never would, because the game client's CPU usage is not zero-sum), your proposal would add a meager 0.7 FPS. Even the most observant of users is physically incapable of noticing the difference between 35 FPS and 35.7 FPS, especially in the middle of a boss fight.
Asking people whose spare-time hobby programming is enjoyed by hundreds of thousands (if not millions) of users to spend time rewriting their addons to use your proposed library -- not to mention introduce a single point of failure by which one mistake in your code, or one day after a patch where you did not have time to update, would mean an enormous number of users unable to run their addons -- just to maybe add 0.7 FPS is simply not reasonable.
There's nothing wrong with wanting to optimize, but I think you're well beyond the threshold of diminishing returns here.