arcade icon indicating copy to clipboard operation
arcade copied to clipboard

Change isinstance checks into asserts or __debug__ blocks

Open einarf opened this issue 3 years ago • 3 comments

We have quite a bit of isinstance testing to sanity check parameters around the code base. For example in collision methods. These are very expensive and we should consider changeing some of the into assert or debug blocks.

# Assert with custom user message
assert isinstance(a, b), "Message to user"

# Debug block
if __debug__:
    if not isinstance(a, b):
        raise ValueErrror("Message to user")

Asserts and the entire __debug__ block will be purge when running python with -O option or the PYTHONOPTIMIZE environment variable is set.

einarf avatar Jun 19 '22 03:06 einarf

This needs to be added to the doc for packaging and development as well. I don't know how to make Nuitka or pyinstaller use these flags, but it seems reasonable for launch scripts for shipped games to include these optimizations:

       -O     Remove assert statements and any code conditional on the value  of  __debug__;  augment
              the filename for compiled (bytecode) files by adding .opt-1 before the .pyc extension.

       -OO    Do -O and also discard docstrings; change the filename for compiled (bytecode) files by
              adding .opt-2 before the .pyc extension.

pushfoo avatar Jun 21 '22 23:06 pushfoo

In benchmarking, I haven't found cases where this is the hold-up. Do we have examples where this is the case?

It makes a big difference to users of the library to catch some common errors however. The 'check for collision' set of commands we have are very easy to get mixed up.

pvcraven avatar Aug 17 '22 15:08 pvcraven

There was a noticable difference when doing lots of collision checks as far as I remember. It's such a low effort change in most situations that won't remove useful feedback for users. We could even consider adding more of them when users can disable it. This is also how the python standard library is doing thing.

Anyway. I see this more as a reminder task. "We can make them into asserts or __debug__ blocks giving the user the option to bypass them... and also lower the bar for adding new ones when this make sense."

einarf avatar Aug 17 '22 21:08 einarf

Most of these are covered now.

einarf avatar Dec 03 '22 05:12 einarf