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

Allow const source buffers when drawing #95

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joewreschnig
Copy link
Contributor

This is one step towards allowing constexpr buffer_ts, saving some RAM
and probably compiling a little more optimally.

@ahnlak
Copy link
Contributor

ahnlak commented May 2, 2023

Can you explain to a "C++ is just C with classes" level why allowing constexpr buffer_ts is desirable (and for bonus marks, how it saves RAM)?

I'm sure it's super obvious to a serious C++ brain, but I'm not incredibly familiar with the subtleties of constexpr.

(other than that; declaring stuff that obviously should be const as const is always a good idea!)

@joewreschnig
Copy link
Contributor Author

Objects marked constexpr (and results of constant expressions generally, but the more you use constexpr the more constant expressions you can easily write) can go into the .rodata executable section, which ends up in flash and not RAM.

This change isn't sufficient to do that for buffers yet (and unfortunately I'm not sure it will ultimately be possible with the current API), but it's a necessary first step.

@ahnlak
Copy link
Contributor

ahnlak commented May 2, 2023

But if you're creating a buffer_t with constexpr content, that data chunk will still be in .rodata - and obviously there are lots of buffer_t instances which are very much not constant.

If it's possible to get there without API-breaking changes I'm all for it - and these changes make sense regardless.

@joewreschnig
Copy link
Contributor Author

joewreschnig commented May 2, 2023

Without these changes, you can keep the pixel data (admittedly, by far the largest part of a buffer) in rodata but not the buffer itself. If the buffer is constant, then the 16 bytes of the buffer itself are also in rodata. There may be a speed hit from loading them from flash, but you also win some back anywhere the compiler now avoids a load completely.

You can do this somewhat easily in C++20 already because it introduces constexpr destructors, and since no such buffer will have .alloc = true it should be safe. (Even some other cases maybe - I'm not up-to-date with the workings of C++20 constexpr new/delete.) Although you are still stuck stripping a const from the pixel data. A wrapper even makes the syntax quite nice by automatically inferring the buffer size from the data size, e.g. after

modified   picosystem/libraries/picosystem.hpp
@@ -43,7 +43,7 @@ namespace picosystem {
       return data + (x + y * w);
     }
 
-    ~buffer_t() {
+    constexpr ~buffer_t() {
       if (alloc) delete data;
     }
   };

then

template <int32_t W, int32_t H>
constexpr picosystem::buffer_t buffer(const picosystem::color_t (&spr)[H][W]) {
    return {
        .w = W, .h = H,
        .data = const_cast<picosystem::color_t*>(&spr[0][0]),
    };
}

constexpr picosystem::color_t sprites[128][128] = { ... };
constexpr picosystem::color_t background[120][120] = { ... };
constexpr auto
    sprites_buf = buffer(sprites),
    background_buf = buffer(background);

I'm not sure how to do it before C++20 without shifting the memory management to a separate, non-constexpr'able class.

This is one step towards allowing `constexpr buffer_t`s, saving some RAM
and probably compiling a little more optimally.
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

Successfully merging this pull request may close these issues.

2 participants