[Freeciv-Dev] [patch #4940] Rewrite universal_fulfills_requirement()
Update of patch #4940 (project freeciv): Status: Ready For Test = Done Open/Closed:Open = Closed ___ Reply to this item at: http://gna.org/patch/?4940 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4940] Rewrite universal_fulfills_requirement()
Update of patch #4940 (project freeciv): Status: In Progress = Ready For Test ___ Follow-up Comment #5: Thanks a lot for reminding me about performance: figuring out how to do this with the lowest call count, least stack usage, and fewest lines of code led to interesting investigations into C. The new patch exposes universal_fulfills_requirement() so that all the requirement_fulfilled_by_foo() functions can be replaced with macros, passes the universal as a pointer, and maintains a static array of the item_found functions indexed by universal kind, initialised in game_init(). Adding support for a new kind now requires writing the item_found function, and then adding 4 lines of source (a savings from the 24 used on trunk). I've added back some asserts: null requirement vectors are fine (iterating over NULL loops 0 times), and if we found a req during an iteration, that req exists, but it is worth verifying that a function to find items exists, and that there is an item to find. Note that actually passing callbacks (rather than maintaining the array) doesn't reduce the code requirement: while one doesn't need the line populating the array, one does need a declaration to be available to the macro, so no net savings (and much wider exposure of implementation details over the codebase). It would be possible to reduce to two lines of code with a MACRO to generate the macros, but I thought that would get closer to unreadable, so didn't go that far. There is an incidental textual dependency on patch #4939, but not a functional one (this patch removes or replaces 6 lines of code changed in that patch(). (file #21404) ___ Additional Item Attachment: File name: rewrite-universal_fulfills_requirement+faster.patch Size:8 KB ___ Reply to this item at: http://gna.org/patch/?4940 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4940] Rewrite universal_fulfills_requirement()
URL: http://gna.org/patch/?4940 Summary: Rewrite universal_fulfills_requirement() Project: Freeciv Submitted by: persia Submitted on: Sun 13 Jul 2014 07:39:18 AM JST Category: general Priority: 5 - Normal Status: Ready For Test Privacy: Public Assigned to: persia Originator Email: Open/Closed: Open Discussion Lock: Any Planned Release: 2.6.0 ___ Details: Patch #4558 introduced universal_fulfills_requirement(), which I've found very useful for patch #4822, bug #14210, and patch #4885 . However, the more I use this, the more I become frustrated with the volume of boilerplate required. The attached patch rewrites the implementation so the callers may use a locally-scoped universal, rather than a malloc()'d universals_u, and determines the name of the item_found function via a switch statement, rather than encoding the function name in the caller. I've chosen to return ITF_YES for unimplemented requirement kinds, given that callers are static, but for safety these could be adjusted to include an fc_assert(). Most of the assertions have also been removed, mostly because I prefer both assert-before use and assignment-in-declarations, which aren't entirely compatible. If others think these assertions are important, I suggest that the appropriate places are a single assert to the passed pointer in requirement_fulfilled_by_foo(), and a single assert on reqs in universal_fulfills_requirement() (and would be happy to add them in another revision of this patch). The result is 8 lines less that need be added to requirements.c for each new requirement kind, with more lines safe for copy and paste. ___ File Attachments: --- Date: Sun 13 Jul 2014 07:39:18 AM JST Name: rewrite-universal_fulfills_requirement.patch Size: 8kB By: persia http://gna.org/patch/download.php?file_id=21385 ___ Reply to this item at: http://gna.org/patch/?4940 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4940] Rewrite universal_fulfills_requirement()
Follow-up Comment #1, patch #4940 (project freeciv): so the callers may use a locally-scoped universal, rather than a malloc()'d universals_u, What prevented thay previously? From the patch it seems that the function did not made any assumptions if the pointer received was dynamically allocated memory or local, though the caller you have adjusted for some reason (or no-reason?) did it with fc_malloc(). OTOH you now change it to be passed-by-value rather than by pointer. I think all that makes is making it less efficient (copying of the full structure in every function call, and maybe also before) and harder to pass something caller originally has as pointer (such as getting memory with fc_malloc()) ___ Reply to this item at: http://gna.org/patch/?4940 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4940] Rewrite universal_fulfills_requirement()
Update of patch #4940 (project freeciv): Status: Ready For Test = In Progress ___ Follow-up Comment #2: I'm not sure anything prevented using a locally scoped variable before, but whenever I tried to do that, I got lost (with either compilation failures or runtime issues). The method I propose is mostly just easier for me to comprehend, and for ease of extension, I think all the callers ought have the same format (ideally we can collapse these into a single function in the future, perhaps with macros for access convenience). Thinking about this in terms of optimisation, the introduction of the runtime switch to determine fulfills is another source of slowness, as compared to the previous implementation. I think I'll try to find another way to reduce the apparent complexity of extending this to new requirement kinds that doesn't have the same performance impact. ___ Reply to this item at: http://gna.org/patch/?4940 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4940] Rewrite universal_fulfills_requirement()
Follow-up Comment #3, patch #4940 (project freeciv): I'm not sure anything prevented using a locally scoped variable before, But making the change to your existing patch to use pointers instead of value should now be trivial. Just take the pointer instead of value in, and change callers to pass local_struct instead of local_struct ___ Reply to this item at: http://gna.org/patch/?4940 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4940] Rewrite universal_fulfills_requirement()
Follow-up Comment #4, patch #4940 (project freeciv): Yes, but the switch statement is unfortunate, and probably causes more of a slowdown than whether we pass a pointer or a struct containing an enum and a pointer. My thought is to replace all the callers with macros that call a common (callback-based) implementation, which should retain the current performance with smaller code-size and greater ease of extension (my primary interest here). ___ Reply to this item at: http://gna.org/patch/?4940 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
