There are all of those directly-accessed globals that get looked up every time the tag gets evaluated which is at least every time those events fire and the unit matches. I guess this isn't exactly OnUpdate, so maybe it's not a big deal. I'm not sure how to deal with it, anyway, as that's just a block of code and nothing around it has any persistence.

I'm declaring all those local variables because it's easier to read when they're used in the formula, eventually; but, wow that's a lot for such a small thing.

So, is there anything I could do better there?

Also, the way I'm using the tag is like this:

I'd love to not have to actually evaluate the tag three times to do that.

Post by Neffi

There are all of those directly-accessed globals that get looked up every time the tag gets evaluated which is at least every time those events fire and the unit matches. I guess this isn't exactly OnUpdate, so maybe it's not a big deal. I'm not sure how to deal with it, anyway, as that's just a block of code and nothing around it has any persistence.

Don't worry about it. Lua is very fast. Global access is totally negligible. We had a conversation in #wowuidev about the supposed benefits of caching the pointer, and with some toying around we actually came to the conclusion that even over several million iterations, the speed gained by caching the function was so small that it was undetectable (because the operating system's scheduler was having a larger impact). So there's literally no gain to be had.

I'm declaring all those local variables because it's easier to read when they're used in the formula, eventually; but, wow that's a lot for such a small thing.

Like I said, Lua is very fast. It's register-based and locals are compiled as direct references to the stack. It's not like OO languages where each of those lines involves creating an entirely new memory-heavy object, and it's not like a stack-based VM where values are copied multiple times.

In fact, the side-effect is that using many locals is often faster. If you've ever done ASM, consider it like CPU registers. When you compute values directly within an expression, it needs to "allocate" a new temporary register to hold them anyway. Doing so inside a local means it does that a step early. This way if you're doing multiple things to the same expression, you often save several CPU cycles involved with copying/moving values.

Eg:

local a = xa = a + 1a = a*aa = floor(a)dostuff(a)

should theoretically be faster than:

x = x + 1dostuff(floor(x*x))

Of course by an infinitesimally small amount, but the point is you don't have to worry about being overly verbose in these types of situations.

So, is there anything I could do better there?

The code actually looks very good. I wouldn't do it any differently.

I'd love to not have to actually evaluate the tag three times to do that.

This seems to be a limitation with DogTag itself. It should cache the values if computed multiple times in the same tag, and maybe it does. In any case, Lua's very fast; don't worry about it.

Post by Wanderingfox

Sadly, I don't think DogTag applications let you declare locals... If so, that would get rid of the 3 calls to frenziedregenheal in the actual dogtag. Though now that I think of it, that is probably just a lookup call, and probably isn't that big of a deal to be done multiple times in a dogtag.

As for the code for the dogtag itself, the only improvements I see off the top of my head is a lot of the locals don't provide any performance gain and can be removed. You only use the value once, which means the local storing it is pure overhead.

Similarly with stam and agi. I'm not too familiar with the cost of local lookups in lua, but if it's non-trivial replacing them directly with the UnitPower calls is probably faster as again you only use them once.

That gets rid of a lot of your locals that don't buy you anything (performance-wise anyway). If it turns out local lookup times are trivial (or close to it) then the proposition above would probably not be worth the loss of readability.

Outside of that, there is only one other improvement I see as even being possible, and that would be caching.

Obviously, the code needs up-to-date information to properly generate accurate results, but there are certain stats that only change on occasion (looking at you agi, stam, and ap). Thus implementing caching for those values, and only changing them when they do indeed change, would greatly reduce the calls to UnitPower (removing 3 of them per update).

The problem there, of course, is how to do that inside of a dogtag, which I have no idea if it is even possible.

editedit: Lua being register-based is actually pretty damn impressive. Had no idea that was the case.

Post by pelf

Well, if caching was really a necessary thing, the function is just a closure (I am assuming it's scoped like I think it is) and I could just pull in a table that's local to the place where I declared the tag to use for that.

As for what could be cached, though ... Stamina is probably pretty constant, but Agility might change with procs depending on what trinkets I'm using. Attack Power is obviously changing constantly by Vengeance. I'm not sure that's worth it ... the caching itself would probably be slower than just asking again. I would guess that Blizzard's unit stat query API functions must be pretty fast, given how much they could be expected to be called.

I like the idea of having faith that DogTag was written with the don't-evaluate-a-tag-more-than-once-in-an-expression idea in mind.

As for the locals .... yeah, if it's not definitely slowing anything down, I prefer the readability.

I think I can, however, remove the watch on the UNIT_MAXPOWER event. I don't think that's relevant here.

Post by Neffi

Leave it in. There may be a one-off case where it happens down the road. As it stands, the event fires so little already that removing it won't have any gain anyway. No downside for "correct" design (following a power event involves listening to both to be 100% correct).

As for caching, it's probably not worth it like you said. After all, the stuff is already sitting in a data structure in C. It's not like the API calls to fetch it again are expensive. You could probably save a little by storing them as locals and using your own events to re-calculate those as necessary, so the tag's function is only ever accessing an upvalue (a local). The gains would be small though.

Post by pelf

No downside for "correct" design (following a power event involves listening to both to be 100% correct).

Why is that? If the math is only interested in the value (a reduction in maximum below 60 would not change the formula), why listen to both? Moreover, in this case, it's just a re-evaluation trigger, really; and I don't have access to any information as to which trigger caused the re-evaluation or any of its parameters, if it had any, from within the tag function.

Post by Neffi

Post by Wanderingfox

I agree. I'm pretty sure the only place any noticeable improvement could be made would be in DogTag itself (assuming it isn't smart about calculating the resulting values from custom tags). Outside of that, it appears any additional gains would be trivial at best, and certainly not worth the hassle of implementing them.

Maybe take a peek at DogTag and see if it does indeed cache the resulting values from custom tags, and if not fix that?

As an aside, does it appear to actually run slow at all, or is this all theoretical? :P

Post by pelf

Entirely theoretical. Plus, I also wanted to chat about code with you and Neffi :). It appears to run quite fast and updates very responsively during fights. Recount is still the absolute worst offender to my framerate but still so much better than Skada as far as its ability to tell me what's going on in a post mortem that I can't do without it. I have to disable it for Elegon, though.

EDIT: You know, I wonder if I could have a tag return a string with color escapes in it already... If I could do that, I could make the output of the tag entirely calculated within. I could have a tag parameter that tells it whether to show it as "HEAL" or "HEAL (OVERHEAL)". Hmmmmmm...

Post by Wanderingfox

I don't see why you couldn't do the color escapes inside the tag result... They're just format characters inside the string, no? Or are you worried about dogtag assuming that they aren't there and adding its own on top of it?

Post by pelf

Yeah I'm going to look into that. I know you can specify the return type so I'm guessing the "text" option might let this work! I will post back.

EDIT: Well, I looked into it and realized that if wanted to internalize the entire output setup, I'd have to implement the SeparateDigits tag's functionality in my tag code. I don't really want to do that. I guess it is what it is!

I did take a look through Parser.lua and it does appear that there's a caching mechanism. I didn't read long enough to understand exactly what he's doing, but honestly, if he didn't implement caching it would be entirely unusable.

Post by Neffi

I'm glad you decided against it. Keep different logic separate. You only ultimately kill the flexibility and complicate the design by doing special-cases like this. On the other hand, your existing code is elegant and functional, and I'd recommend publishing it as a patch to DogTags, or a separate plugin.

Like a certain infamous BSD developer once said, it doesn't matter how fast your code runs if it doesn't run correctly. For the majority of users, hardcoded coloring and formatting is both incorrect and frustrating.

Post by Wanderingfox

Post by pelf

Publishing it as a DogTag library is an interesting idea. I could see polishing that one up, but I am not sure what else might go with it. I'd hate to publish it with just one tag in it :). As it is, now, I'm actually running the register line in _DevPad.

Post by pelf

DogTag:AddTag("Unit", "NextFrenziedHeal", { code = function(factor) if GetShapeshiftForm() ~= 1 then -- if not in bear form, don't calculate -- even when shifted out of form, the game still reports a rage value return 0 end

Post by Neffi

Renamed it to be a bit shorter and to match the other one I added.

I don't know if you can add abbreviations for tags, but I'd recommend an abbreviated form like LastFrenzHeal. I doubt I'm alone in finding long variable names annoying, especially so in tag environments where it might appear multiple times on the same line.

Added the ability to specify a factor to support the 4pc T14 bonus. I just pass 1.1 into the tag like .

I'd make that automatic. I'm not sure if there's an easy way to get set bonus info. If not, no big deal.

Global variable so that the DogTag can access the value when it's rendering.

If the event handler and tag register are defined in the same file, you can make that local. I'd recommend it. If you don't want to, at least prefix the variable name with something unique, like PDT_LastFrezRegenHeal. It's not unreasonable to assume another addon may also be using a variable named like that.

I'm determining if I cast it with a GUID comparison rather than bit.band and the MINE flag. This seemed easier.

While bit.band should be very fast, GUID comparison will be faster. Using bit.band essentially amounts to one extra logical AND operation (one for the bit.band call, and one to check its result for validity). GUID comparison only has to perform the one AND operation (simply comparing the two numbers ). Granted the difference is exceptionally small, but I feel GUID comparison to be both easier to read and more logically suitable. Unit flags might change in the future (they have before), and they're really there for doing more flexible checks, not just straight unit comparison.

Publishing it as a DogTag library is an interesting idea. I could see polishing that one up, but I am not sure what else might go with it. I'd hate to publish it with just one tag in it :). As it is, now, I'm actually running the register line in _DevPad.

Maybe think about what other things druids might care about, and add a few more tags? You could release it as something like "LibDogTag-DruidUtility".

Edit: I don't use DogTags, but I do have a druid. If I think of some ideas that'd be useful as tags, I'll code them up and post them here (license free, so you can do as you please).

Post by pelf

I don't know if you can add abbreviations for tags, but I'd recommend an abbreviated form like LastFrenzHeal. I doubt I'm alone in finding long variable names annoying, especially so in tag environments where it might appear multiple times on the same line.

You can. I could make LFH an alias for it. That's not a bad idea anyway; it would make the set of tags that use this one a lot easier to read.

I'd make that automatic. I'm not sure if there's an easy way to get set bonus info. If not, no big deal.

Well, I'd probably have to check the 5 tier gear slots for 3 item IDs per slot (LFR, Normal, Heroic) both when the game loads and when a gear change happens, right? That seems arduous.

If the event handler and tag register are defined in the same file, you can make that local. I'd recommend it. If you don't want to, at least prefix the variable name with something unique, like PDT_LastFrezRegenHeal. It's not unreasonable to assume another addon may also be using a variable named like that.

Not sure where my head was. Obviously it can be local :|. Fixed.

Maybe think about what other things druids might care about, and add a few more tags? You could release it as something like "LibDogTag-DruidUtility".

Edit: I don't use DogTags, but I do have a druid. If I think of some ideas that'd be useful as tags, I'll code them up and post them here (license free, so you can do as you please).

Most of the standard Druid stuff is covered in the LibDogTag-Unit library. I'm currently just using functionality in TellMeWhen to get the value of my Vegeance buff, but I could easily see making a tag.

EDIT: I got some advice on some of this from the #wowuidev channel while I was writing it. Several of them expressed amusement that anyone was still using DogTags to begin with. Heh. I guess most of the hip devs have moved their projects to Luatexts at this point. I imagine many authors are just locked into DogTags for compatibility with their users' configurations, though. How annoying would it be for some author to just change that... Anyhow, TellMeWhen does still use them, at least.

Thanks for the feedback :).

EDIT2: Here's the full set of tags for the heal/overheal text:

Post by Mike

Post by pelf

Well, two possibilities: (1) you can create a small addon to add the two DogTags, or (2) you could do what I'm doing and add them with DevPad. The latter is what I did for simplicity, but once they're set up, the two are equivalent, more or less.

EDIT: Speaking more generally on topic, I'm going to add a tag, as well. That one has a straightforward implementation.

However, I'd also like to have a tag. That's a little less straightforward. The value of Vengeance is available from one of the variable returns from UnitBuff. The problem is tracking it changing. If code weight wasn't an issue, I'd watch the UNIT_ATTACK_POWER event and check the value from the Vengeance buff in there. However, attack power changes a lot in combat -- mostly from Vengeance changes, but also from other sources. Do you think that's going to be too much? The problem I see with throttling it somehow is that the max value could be missed while in the time span between updates.

Also, I'd like some way to reset the saved values when I want to. I suppose I'll register a slash command for it (can't think of a good name at the moment) and have its parameter be what to reset. It would default to all, but would accept one or more of "lastfr", "maxfr" or "maxv".

Finally, I remembered why I needed the value to be global. In TellMeWhen, my show condition for the "last" value is Bear Form and...(DTS_LastFrenziedRegenHeal and DTS_LastFrenziedRegenHeal > 0)

EDIT2: I implemented the tag and the saved values reset slash command. Still want to hear what you think about the Vengeance one before I move on that. The code's getting a little long to just paste in here every time, so I threw the current state of it into a gist: https://gist.github.com/4647348

Post by Mike

Ok so I got devpad. I have little to none coding experience, although I can usually read and edit code, I can't write it.