Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
306 Code refactoring, performance [2015/11/07] [Utumno]
This release features an almost complete refactoring of the codebase, fixing many bugs and opening the way to extend Bash functionality. Bash, being a community work, has over the years become an assortment of hacks, patches and cruft and the program was just about to become non-operable. Example issues - in no particular order (although probably the worst was the horrible performance): - deleting an item in the lists displayed by Bash ran different code depending on whether the user hit delete or right clicked and chose delete - most of those pieces of code were buggy - start up took more than 15 seconds on large Oblivion Load Orders and alt tabbing out and back into Bash would take 6-7 seconds - last one should take no time in the usual case - as seen from the (user visible) debug prints the game detection algorithm run 3-5 times - many, many broken things - including performance in general (most simple operations would hang Bash for some seconds), display glitches and general UI inconsistencies and internal data structures corruption (see #176) Those bugs reflected the state of the code - again in no particular order: - bosh.py (the data model) in 305 was 24635 lines (reduced from 30550 lines in 304.4 - see bosh section below) - basher.py (the UI lists/menus etc) was 18936 lines - the seven tabs that Bash featured were backed up by Tank (2) and List (5) instances - so implementing a feature (say delete) needed edits to at least two classes (usually 7) - the wx library was intermingled with the basher code - the menu items (Links) defined 215 AppendToMenu methods with nearly identical wx code - readability suffered - most attributes/variables were named `data` or `items`, globals hacks crippled the IDE static analysis, copy paste (aka ravioli) code was everywhere. When I started in it I had no idea if refactoring the code was even possible, core Bash is 80k lines of code. It was clear that bosh needed splitting, that candidate number one was the patchers code (well defined and functional) and that basher should follow - but was not at all clear if those were possible, let alone how exactly should the code be structured and how much time would this take. Turns out that it was possible: Bash was indeed coded with modularity in mind (by Wrye ?) but later additions bloated the initial modules beyond recognition and broke the (non enforced) encapsulation - however it wasn't too late. Also turns out that refactoring needed 2 full work years of a single engineer (learning python). The huge performance boost that this release features is the measurable effect of the increase in the code quality. In the items below I try to describe in a quantitative manner what "code quality" means and how it was increased. That's meant to be read from an app that links to the commits - both github and gitk do but additionally github links to the issues and files. ### bosh This release features splitting the patchers' code out of bosh to a dedicated package (whose final structure is still at stake). This reduced bosh to 10516 lines while naturally exposing class dependencies issues, hidden by the "one file program" nature of Bash. The attempt to split bosh had already started in 305, involving: - an effort to eliminate code duplication in C and P patchers using multiple inheritance (chopped off around 1500 lines of pure copy paste code) - see commits in 3c40989 - splitting out what was to become `record_groups.py` - see c3e3543, but don't imitate it - lots has changed since then, including import conventions and commit style - for instance commits on dev that do not launch are strictly forbidden. - splitting out what was to become `parsers.py` - see commits in 08ab6e2 Final (`record_groups.py`, `parsers.py`): 6ae30fc With those out of the way it was finally possible to rip the patchers out of bosh - starting here: 983f259 That was not a mere copy paste rip - warnings were squashed, long lines were wrapped, and most importantly a lot of tricky refactoring was applied to eliminate code duplication - see for ex. commits in: 72fc8a0 for ripping and commits in (note the negative line count): 5d21377, fdab163, 78a85e0 for refactoring See #3 for commit links (finally closed in b6b743b) and #124 for the (still open) package layout issue. Be aware that a lot of things changed since, including import style and the overall package layout - see: e6b2e0e The remaining bosh.py code had some preliminary refactoring applied to it (archives handling, BAIN error handling and refresh etc - see 6ddd4a8) but core bosh refactoring will take place in 307. Major goals are splitting bosh into a number of modules, getting rid of old style classes (DataDict hierarchy, that is the data store singletons) and refactoring settings to efficiently support profiles. ### .data There are 544 occurrences of `.data` in 306 (1081 of `data`) as opposed to 1071 in 305 (2221 `data`) - baring string literals and comments (Pycharm only recently made these scopes available in IDE search, they were much needed). Practically one had to run the debugger to see the type of the object one had at hand, since the variable/attribute name was always 'data': - data is the wrapped dictionary of `DataDict`. Most accesses to this dict were not via the wrappers the DataDict provides. This took many commits to fix - ex 56198be. Before even _thinking_ "performance" read the performance section below (no, none of those modInfos.data.keys() calls was in the middle of a tight loop). - data also was an attribute of the Link hierarchy which had _different meaning based on an isinstance check_: 92e50cd. It could either mean a DataDict subclass (so in Links code there were `self.data.data` accesses) _or_ the list of selected items in the UI, leading to a complete chaos - see the commit above up till the final removal in ffae59e. - UIList subclasses have a `data` attribute that points to the DataDict singletons in bosh. That was dealt with throughout coding 306 as it is closely related to a deeper issue namely the intermingling of UI and model/business logic code. This stems from the 'one file program' nature of Bash so solving this properly means refactoring is at 100%. - to somehow improve readability I introduced idata and pdata attributes for the Installers and People data links - eba53f9 - Those are transitory and meant to also help in eliminating Tank. The links of List subclasses were using self.window.data, whose uses I strived to encapsulate - random example: 0934f1f - all kind of different classes attributes were named `data`. Those required dedicated commits (hey, I couldn't just do a search and replace for "data") - ex. 9839c47. An IDE really helps here. - finally data local variables were renamed - ex. 8e77283 The war with data was going on in parallel with smaller scale wars with `items` (e87dca7 and its parents), `refresh` (4d872ab), etc. ### basher (#163) This is the part of refactoring that I consider almost done - and it was a big pleasure coding it. Since I first tried coding for Bash the dichotomy between balt.Tank and basher.List clearly stood out as a major issue, as did the omnipresence of the wx library and of course the sheer size of basher.py (the UI module). Basher became a package and then the UI API went through a complete rewrite. Basher globals served as a guide in the splitting process, and a metric of class dependencies. Eliminating those globals was an ongoing effort throughout 306 coding - see for instance: 892a19c (screenlist, statusBar), 5852328 (gInstallers), faea771 (modList) till final elimination in bab7c00 Guest star: bosh.links (353be43) Globals served as a link also between Bash's modules (so they evince class and module coupling issues) - eventually encapsulated as static fields of the balt.Link.Frame singleton - see 0690cf3 Link.Frame is an interesting binding of the UI (basher) and the rest of bash that will serve as guide to a later refactoring phase. Especially Link.Frame.modList use in bosh must be eliminated but this is related to the RefreshUI/delete API finalization. The items below detail the UI refactoring. #### wx wx usages went down by an impressive 1148: 305: 2260 usages (balt: 381, basher: 1586) 306: 1112 usages (balt: 418, basher/: 494) Note balt barely going up. This is because, as was stressed in the relevant issue (#190), decoupling the wx library from the code does not simply mean "create wrappers for wx classes/constants and drop them into balt - it means that balt must export an API", so: - balt should not return wx items on which wx methods will be called - see: 9856c69 for example fix. - the code outside of balt should be agnostic of wx ids. Bash code was far form agnostic of wx ids - on the contrary it used them extensively: - balt was exporting it as a function parameter - this mostly resulted in client code calling it with `wx.ID_ANY` (adding wx usages) - see for ex. a555160, 18cf2b1 up till finally dropping the last one of them in 370bef3. - event handlers were using `event.GetId()` - for example fix see 081bfa6. - Link.Execute() has an event parameter which was mostly used with the IdList hack to define lists of menus - this was taken care by the ChoiceLink subclass, introduced in 6fbec32 till finally the last uses of IdList were binned: 000f320 and IdList was removed (967b921) - note that this permitted dropping the `_id` parameter from `Link.__init__`. ItemLink.Execute() `event` parameter is in fact _never used_ in the 306 Link API iteration and will be removed. That's "decoupling wx from Bash code". For commits that combine both fixes see: 46599bb, 9a77bfc The bulk of what remains is to hash up a clever interface for sizers - needs thought as it must accommodate all usages while being powerful enough to cater for new ones. That being said, another thought is incorporating some basher wx dependent stuff back to balt which anyway should be split to a package - at least its bosh depended classes should be clearly separated from its "abstract" wx wrapping API. #### Link API In a gigantic merge (054970e - avoid) the Link subclasses were ripped from basher to per-tab modules - this is not final but it was a necessary step to start splitting basher. As with the rip of patchers this was not a copy paste rip - a new Link class API was introduced. Unlike the patchers rip however (where package structure and the class hierarchy is still at stake) the Link API has few rough edges left in. The guide for designing the Link hierarchy was `AppendToMenu()` calls that featured boilerplate wx code - for wip boilerplate elimination commits see: 6cf40c1 where `EnabledLink` is introduced and 658fcac where my favourite `TransLink` is introduced (AppendToMenu could become quite _complex_ - see also `ChoiceLink`: 6fbec32). The merge chopped off 785 wx uses and AppendToMenu was confined in balt, and reduced to 8 different implementations vs 215 (!) in 305. The Link API significantly evolved since - compare the 306 (semi final) form with the 305 implementation: 305: https://github.com/wrye-bash/wrye-bash/blob/d8f05202e6485a3bef0d92bb55a731b8040eb94e/Mopy/bash/balt.py#L2098-L2114 306: https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/balt.py#L2392-L2446 Previous (old style) Link class explicitly defined _different_ attributes based on an isinstance check, checking if the underlying window was a List or Tank instance. This made the code plain unreadable (given also the names used, see .data section above) while enforcing the Tank/List dichotomy. The isinstance check was finally removed here: 6d260f0. Here is `INI_ListErrors` Link before and after the refactoring: 305: https://github.com/wrye-bash/wrye-bash/blob/d8f05202e6485a3bef0d92bb55a731b8040eb94e/Mopy/bash/basher.py#L11168-L11191 306: https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/basher/ini_links.py#L71-L90 Note AppendToMenu code is encapsulated in EnabledLink, wx wrapper introduced for the clipboard, text and help are now class variables, "data" is now "selected" and the lines are wrapped for more readable, declarative and concise code. For a more complicated example see: 305: https://github.com/wrye-bash/wrye-bash/blob/d8f05202e6485a3bef0d92bb55a731b8040eb94e/Mopy/bash/basher.py#L11029-L11122 306 :https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/basher/mods_links.py#L84-L161 Previous implementation directly manipulated wx IDs (by global ID_*** complex hacks) while the new one declares classes and lets ChoiceLink superclass take care of creating and appending the menu items. This permitted focusing on the Execute code which as seen got improved while refactored to remove duplication. This class still has rough edges (\_self...) which will be corrected in 307 along with rough edges in the API - for one dropping ItemLink.Execute() `event` parameter. Of note that the Link subclasses featured a lot of duplicate code apart from the wx related stuff - see for instance: 1a9a29e for duplicate code in Mod_Export Links. #### UIList After the Links were ripped from basher it became obvious that refactoring could not progress while balt.Tank and basher.List were both still around. That lead, one fine morning, to a new class - UIList: 1462227. Both Tank and List had an attribute (glist and list (!) respectively) pointing to a ListControl instance (the items displayed in the UI). The first UIList merge: abd5f24 (see commits there for how UIList was engineered from common Tank/List code) has List.list removed in favour of gList: fc7bde8, to be finally encapsulated as _gList in c48ce7d (note there is a completely unrelated `gList` patcher attribute). ##### UIList.SortItems() The first real snug though was unifying the sorting API of Tank and List. ListControl does not support sorting, one has to introduce a hacky dictionary mapping indexes to displayed items' ids (not wx ids) - that's what Tank was doing (undocumented !) while List was using no less than PopulateItems - resulting in bad performance most apparent in the ini tab, where sorting would stat the file system (so clicking on a column header Bash would hang couple seconds). I proceeded unifying the List.SortItems() overrides (6f0adc9), then moving the Tank mechanism to the ListControl class where it belongs (52a4a22) and finally moving SortItems to base: 42d213a - note OnColumnClick callback to base and ignore the isinstance(self, Tank) - corrected later. This made also various other fixups around the Sort API possible, namely adding sort indicators in all Bash lists (only mod tabs had them), fixing master lists sorting (and display) and the beginning of TankData param API deprecation: 7a3b872. ##### UIList.RefreshUI() Second snug and a blocker for fixing performance was centralizing (and cleaning up) the RefreshUI family and underlying PopulateItem(s) (UpdateItem(s) in Tank). Some commits: - Moving RefreshUI to List: 39e1e60 - List.PopulateItem(): f390ab7, ecf30e9 - UIList.PopulateItems(): 350075a - Tank and List `__init__()` eliminated: a510b23 Unifying the RefreshUI API made finally possible to remove List itself: d94a09c. More work is done - for instance see some work on refreshing the details displayed in: 3ff6cf2. ##### UIList.DeleteItems() Once List was no more and Tank a relic of itself it was finally possible to tackle the delete API. First relevant merge is: 02a5891 but the API is yet in alpha due to calling UI code from bosh - see: 6452bcb and its parent merge containing 3da60e6 (the ugly DataDict.delete_Refresh()). This API will hit beta once the data refresh methods return deleted, modified and added - see how this precious info is currently discarded: return bool(_added) or bool(_updated) or bool(_deleted) https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/bosh.py#L3887 ### Performance (#123) As seen by profiling, the performance bottleneck was the libloadorder32.dll we bundled with bash. Given the central place load order operations have in a mod manager application, the introduction of a laggy dll had taken Bash's performance down on its knees since 51f99f2. But this was far from being the only problem. Due to the spaghetti nature of the code (throw some event programming in) Bash was very generous with its refreshes: - it would refresh its data three (3) times on boot - RefreshData callback was triggered whenever Bash took focus - _including_ after Bash itself displayed a dialog to the user - on Oblivion, while Bash had refreshed its mod infos it was _always_ requesting also a refresh from libloadorder, to be sure - so, for instance, when just tabbing out and back in to Bash (with absolutely no changes in the Data dir) libloadorder was triggered and went through its refresh cycle - see ad05f44 for the fix - refresh in BAIN, say, install operations was called twice, along with a double call of modList RefreshUI - refreshing the mods panel would result in also refreshing the saves panel - even when the change that triggered the refresh did not affect saves - see commits in: 63d8dec Now, given that most of those refreshes ended up triggering a refresh from the libloadorder32.dll, Bash would take of the order of 10 seconds for all but the most trivial operations. But even if libloadorder was lightning fast the double and triple refreshes were as much a bug as anything - so the dll lag was even a blessing in disguise as it made apparent the underlying chaos. To solve the dll issue one alternative would be to reimplement it in python. But given that the same dll is used by other mod managers and related apps I anyway had to know what's in there. So I ended up forking libloadorder and going through the C++ ~~pain~~ code: https://github.com/Utumno/libloadorder/ I realized that some of the operations performed by the dll are inherently costly and would actually need a complete rewrite using caches - for instance the mod validity checks (see Utumno/libloadorder#3), which by the way involves yet another library and is not clear what is checked (as is not clear what Bash checks and if those checks are basically performed twice - Utumno/libloadorder#6). As the situation was critical in Oblivion mainly (due to stating the filesystem for mod times) I explicitly bypassed the dll because I anyway have all the data needed to determine the load order - 7cd6533 - while I still use the dll for saving it. Liblo 7.6.0 commit (f5de6c8) apart from performance fixups fixes some other issues with liblo 4 and 6 like writing loadorder.txt when not there, return active plugins in order, adding files to libloadorder.txt that are added to Data, etc - see: Utumno/libloadorder@7bb5cfb But rewriting the dll was not enough - on the Python side of things the load order internal API was a giant hack, complete with event handlers and absolutely no encapsulation, let alone proper caches. I introduced a new module `load_order.py` (for first iteration and design decisions see 98ed549) to centralize load_order handling - will be finalized in 307 but already Plugins is again encapsulated in ModInfos which serves as the basic internal load order API for the rest of Bash. Load order API will be final when implementing features such as "undo load order change" is a breeze, but already makes implementing minor load order related features much easier: 14ecafc. Using the caches I introduced I was able to fix performance of getOrdered() - was O(n^3\*logn) instead of O(n\*logn): 5d7ed0b The double and triple refreshes were a deeper issue - the "one file program" thing - so rewriting refreshes (RefreshUI, the DataDicts refresh, RefreshData etc) - was an ongoing effort throughout 306 development, going hand to hand with the UI refactoring (see #163 for RefreshUI and #123 for links to some of the most relevant commits). In short: - the refreshes on bash dialogs were taken care using a decorator to unbind RefreshData(): a71f3f2 - the BAIN issues were addressed finally here: 8107f7b - BAIN however needs to be split in a dedicated package to start properly fixing it. Goals for the next few releases: - cache ini reads - finalize then profile the load order handling API - profile the refresh of data on boot and optimize it - centralize refresh of data and UI so we can eventually - switch to an event based model of runtime refresh (watchdog ?) ### Warnings Pycharm code inspection emits 2228 warnings in 306 as opposed to 4229 in 305. Metrics are from Pycharm 4.5.4 - using this inspection: https://github.com/Utumno/wrye-bash-pycharm/blob/168253bca81313a3cc7dc85ee53747b116e984fb/.idea/inspectionProfiles/default_copy_no_typos.xml on this scope: https://github.com/Utumno/wrye-bash-pycharm/blob/168253bca81313a3cc7dc85ee53747b116e984fb/.idea/scopes/wrye_bash_all_no_cint.xml Warnings come in various sizes and shapes but in general show low code quality. Some I batch fixed (like 'Comparison to None performed with equality operator') but in general I was fixing warnings when visiting nearby code so the remaining ones mostly mark code areas that need attention. On the other end of the spectrum there are warnings that deserve issues of their own (like 'Too broad exception clause' - nicknamed the most diabolical python anti-pattern - see #200). Other interesting cases were: - "Function 'close' doesn't return anything" led to a rewrite of the archives handling code: c29d7b8 - copy paste unused variables were kind of a guide to otherwise heavily undocumented code - but also often pointed to bugs - "method may be static" (264 in 305, went down to 72 in 306) is a classic example of a warning one fixes while editing the code - the remaining ones should be revisited for 307, along with their classes. - unused imports are on their way to extinction - the remaining cases involve `import *` patterns and normalizing the import style, which shall be done when package layout is frozen - and the ugly `__all__` directives one has to update on adding a menu item: 6a2e4a9 - UILists and their links belong together. ### Cruft (#173) Cruft removal got a dedicated issue so returning developers could readily track when and why code they were familiar with got removed. Cruft is not just unused classes, commented out code etc - it is also compatibility with ancient Bash settings (will be removed in 307), non working tabs (like the PM tab also to be removed in 307) and compatibility code with pre 2.7 python (see 6d0a944). One major piece of cruft removed in 306 was BALO: b520591. As you can see removing BALO was needed for tackling refresh - as it was a complex, obsolete piece of code that greatly complicated both refresh and load order handling - and even the Link API (see notoriously complex Mod_BaloGroups(ChoiceLink) subclass here: aa5b89a). ### Formatting & wrapping After some discussion it was clear that PEP8'ing the code would be a pointless effort at the state it was in - however word wrapping is a _functional_ requirement and 79 chars is well thought of, even if you use a wall projector for programming. 305: Total number of non-blank lines: 82463 in 29 files 96.22 lines less than 80 characters (3120 lines exceed) 98.63 lines less than 100 characters (1130 lines exceed) 99.44 lines less than 120 characters (462 lines exceed) 306: Total number of non-blank lines: 85366 in 63 files 97.89 lines less than 80 characters (1803 lines exceed) 99.26 lines less than 100 characters (631 lines exceed) 99.71 lines less than 120 characters (248 lines exceed) Note: - I do not include `cint.py` (and the `chardet` package which should become a submodule) - a big part of the line wrapping was made by manually correcting Pycharm's formatter - so have a close look - the increase of line count is mainly due to 21de440 which adds 5130 lines most (4974) of it being static data in Mopy/bash/game/skyrim.py. Considering there were 34 files added for 34 * 23 = 782 lines of licence text and that wrapping should have produced another 1k lines at least _core Bash code was actually reduced by ~4000 lines_. That is more than welcome and should be imitated - _less code more features_ is a great motto. ### Boot Booting process got a facelift to be finalized in 307. This involves game loading (see 97fc9cf), fixes to the game detection algorithm (e19237c) which used to run 3-5 times on boot, a fix for our custom importer (43e359a) compliments of @mjpieters, and finally a reworking of boot error messages (8dbdd49). ### WINE As reported in #203 "WryeBash v304.2 works beautifully in Wine" - the windows only shell UI stuff broke it though. One of the last things I did for 306 was fixing this - now Bash runs again on WINE: 124f314. Still making it run correctly and in a second phase making Bash run _on bare Linux_ is a code quality goal that got dedicated issues (#240, #241, #243) ------------------------------------------------------------------------ This release is dedicated to Wrye. Special thanks to: - @Sharlikran, @Supierce, @lojack5, @Arthmoor, @alt3rn1ty, @psilocide (aka lefenger), @leandor for testing, support and sharing their knowledge, - @jfpelland, @wduda, @Isenr and @valda for patches, - @mjpieters for saving my sanity at SO a couple times as well as @zed for helping me rewrite the archives handling code. ------------------------------------------------------------------------ # Conflicts: # Mopy/bash/bass.py @@@ -21,9 -21,42 +21,46 @@@ # https://github.com/wrye-bash # # ============================================================================= """This module just stores some data that all modules have to be able to access - without worrying about circular imports.""" + without worrying about circular imports. Currently used to expose layout + and environment issues - do not modify or imitate (ut).""" + import os as _os + import ConfigParser as _cp language = None <<<<<<< HEAD +AppVersion = u"304.4" ======= + AppVersion = u"306" + bashIni = None + + #--Null strings (for default empty byte arrays) + null1 = '\x00' + null2 = null1*2 + null3 = null1*3 + null4 = null1*4 + + def GetBashIni(iniPath=None, reload_=False): ##: needs work + iniPath = iniPath or u'bash.ini' + global bashIni + if reload_ or bashIni is None: + if _os.path.exists(iniPath): + bashIni = _cp.ConfigParser() + bashIni.read(iniPath) + return bashIni + + class Resources: # this belongs to basher but leads to cyclic imports, so... + fonts = None + #--Icon Bundles + bashRed = None + bashBlue = None + bashDocBrowser = None + bashMonkey = None + + # move with its uses to a cross platform 'env.py' module - maybe add bashIni + try: + import _winreg as winreg + except ImportError: + winreg = None >>>>>>> rel-306 ------------------------------------------------------------------------ Build with `2.7.10 (default, May 23 2015, 09:40:32) [MSC v.1500 32 bit (Intel)]` Closes #187.
- Loading branch information
