Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code refactoring: edit various components to make them conform with Python idioms and to optimize for speed #70

Open
josephsl opened this issue Jul 16, 2020 · 2 comments

Comments

@josephsl
Copy link
Contributor

Hi,

The following will require extensive testing as it concerns optimizations:

Although Braille Extender works beautifully, there are many places where it can benefit from code rewrite to conform with Python idiom, syntax suggestions, and optimize for speed. For example, when one obtains an overview of the currently active input and output braille tables, a function named utils.combinationDesign will be run. Part of its job is to convert braille dots to a representation suitable for output. At the moment a while loop is used, but it was found that z for loop or a subtle optmization would benefit readability and speed.

Note on speed: in order to truly test the effectiveness of speed optimizations, one must:

  1. Benchmark the code path in question multiple times (preferably with time.time() and friends).
  2. In order to truly optimize for speed, bytecode disassembly is a must.

To illustrate the second point, in combination design function found in utils module, each iteration of the while loop will do the following:

  1. Before while loop is invoked, dot (index) is set to 1.
  2. Current dot value (represented by the variable "i") will be converted into a string, not once but twice (first to obtain the dot, second time to check membership).
  3. The eventual result string (represented by the variable "out") will be concatenated either with the dot value or unicode for dots 3-6.

The function takes 31 Python virtual machine instructions. Converting this into a for loop results in 31 instructions but with the following change: instead of assigning the variable i to 1 in the beginning, it is incremented as part of the for loop with the range set to (1, 9). This makes the code cleaner.

But there is an optimization opportunity: what if instead of treating the dots as integers, treat them as a character? This results in:

  1. Instead of a range-based for loop or a traditional integer-based while loop, the variable i will iterate over a string (string is a container as far as Python's iteration protocol is concerned).
  2. No more integer to string conversion as data can be compared directly.

This results in the function using only 25 Python virtual machine instructions. Because combination design function will be called whenever a dot combination exists in the input and output table, this optimization results in savings of hundreds of bytecode instructions. In order to do this kind of optimization, bytecode disassembly is a must.

There are many other places where this kind of edits would benefit the add-on, not only for code readability but also for future maintenance. As for when the accompanying pull request will show up, it won't be done until #69 is done.

P.S. while at it, I propose adding source code comments (finally).

Thanks.

@AAClause
Copy link
Owner

Hi Joseph,
Thanks a lot for all your issues/suggestions.
Know that I'm very glad to receive your comments.
Indeed many things can be optimized.
Some portions of the code come from my beginnings in Python and are unfortunately not optimized/very poorly written. I'm totally agree to change that. :)
Currently, I am actively working on #56 and #64. But I am OK to work on this after those (probably from August).
Feel free to make pull requests/comments!

Thanks again and good luck with my code :)

@josephsl
Copy link
Contributor Author

josephsl commented Jul 16, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants