is_..._in_range() with a standardized fingerprint
alienvalkyrie Question is what that fingerprint should be.
The option closest to what we currently have would be
with the universals_u union passed directly (since I'm reasonably sure declaring each function to just take the type it wants would be very risky).
On the other end of the spectrum, a more compact (function-type-wise) option would be
but that would lead to a lot of boilerplate between assertions and unpacking the data that every variant will need.
- enum fc_tristate
- is_foo_in_range(const struct req_context *context,
- const struct player *other_player,
- const struct requirement *req)
- {
- fc_assert_ret_val(req->source.kind == VUT_FOO, TRI_MAYBE);
- const struct foo *pfoo = req->source.value.foo;
- enum req_range range = req->range;
- /* determine if req is active, *ignoring* req->present */
- }
If we go with the latter, it might also be sensible to rename the functions as is_foo_req_active.
Why other_player isn't part of the context? (I have a feeling that I've asked this before, or even figured it out from the code myself, but fail to remember now)
Passing requirement pointer instead of some fields from it would have the benefit that when requirement structure is changed (new fields added or whatever) we would need to change directly affected callbacks only, not all of them. And typical callback does not need to do much unpacking; if 'range' is used just once, we may as well access it directly "switch (req->range) {" instead of "enum req_range range = req->range ; switch (range) {" (difference is only in source code readability anyway, compiled code will be identical). Agree about the function naming.
alienvalkyrie Reply To cazfi
Why other_player isn't part of the context? (I have a feeling that I've asked this before, or even figured it out from the code myself, but fail to remember now)
Actions code, and future considerations. Action enablers need two of of everything anyway (actor and target), so that just uses two contexts (without the redundancy of specifying each player twice); and there is also the option to do something similar for other things with requirements in the future, turning other_player into a whole second context (which I previously wrote about here).
Reply To cazfi
Passing requirement pointer instead of some fields from it would have the benefit that when requirement structure is changed (new fields added or whatever) we would need to change directly affected callbacks only, not all of them. [ ... ] Agree about the function naming.
Yeah, it seems like the reasonable option. I'll look into doing that then.
alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie Have you been able to run any profiling for this? To see that this really has the effect we hope (or at least; that there's no factor that we have missed that would cause this to be less efficient)
alienvalkyrie Reply To cazfi
Have you been able to run any profiling for this? To see that this really has the effect we hope (or at least; that there's no factor that we have missed that would cause this to be less efficient)
I have not. I was going to run some autogames today, both to make sure I didn't break anything, and to get a look at the turn change times.
alienvalkyrie Reply To alienvalkyrie
Reply To cazfi
Have you been able to run any profiling for this? To see that this really has the effect we hope (or at least; that there's no factor that we have missed that would cause this to be less efficient)
I have not. I was going to run some autogames today, both to make sure I didn't break anything, and to get a look at the turn change times.
Looks like nothing broke, but it doesn't seem to be consistently quicker or slower. Though I did just realize I ran it on an unoptimized (debug) build, so I'm not sure that means much.
@cazfi if you want to do some proper profiling, feel free to take over this ticket; I don't currently have the necessary builds and tools set up.
For the record, I do think this change is already worth it for the sake of abstraction and consistency, irrespective of efficiency gains; so it would still be justified to push it even without profiling data to back it up.
Reply To alienvalkyrie
For the record, I do think this change is already worth it for the sake of abstraction and consistency, irrespective of efficiency gains; so it would still be justified to push it even without profiling data to back it up.
Agreed. I'll try to do some profiling for it at some point, but there's no need to wait for those results before pushing this in.
To have sensible comparisons of two runs (with and without the patch), I would need a moment when there's no other heavy processes going on the same machine and interfering. Usually there's always a couple of builds and/or autogames going.
alienvalkyrie First result, from a civ2civ3 (default ruleset) autogame.
With S3_1 commit 586dd262d4 (just before the patch), is_req_active() took 4.26% of time, or 56,26 seconds.
With S3_1 commit 696ee7bdc1 (with the patch), is_req_active() took 2.40% / 32.03 seconds
Of course, we get what we measure (i.e. time is_req_active() takes - not overall performance). It's possible that earlier version had lots of stuff inlined to is_req_active() and the time spent to those functionalities have just been moved to called functions in the new revision. There's an *increase* in the overall execution time (but that's complicated matter, so I want more data before concluding anything)
Profiling results from three autogames, before and after the commit, attached.
Looking to make is_req_active()
I think the main problem is that those 'case's in the switch are different from each other. The compiled code really need to go "if else if else if else if else if else" to find the block for the requirement type they are interested about. It would be much better to have all cases to call similar callback function -> then we could have those callbacks in an array indexed by the requirement type id.
With the new req_context this makes even more sense than before. is_req_active() can just pass the context pointer as it got it, to the callbacks. There's no "passing parameters needed by the particular callback"
The req_context point makes it clear that the change won't get backported to S3_0, where we don't have that context.