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

Even small programs can be too big #4934

Closed
jaustin opened this issue Feb 9, 2023 · 15 comments
Closed

Even small programs can be too big #4934

jaustin opened this issue Feb 9, 2023 · 15 comments

Comments

@jaustin
Copy link
Collaborator

jaustin commented Feb 9, 2023

Describe the bug

With MakeCode V5, we know some programs can get quite large, but until now I thought this was really limited to things that used extensions. See #4788

However, here's a very small program that still won't fit on a V1.
https://makecode.microbit.org/_bfs6mzETmeUi

image

To Reproduce

  • Build a program as above
  • Try to download onto a micro:bit

The start melody block is definitely part of the problem, and we know from #3868 that there was a bit of a longstanding issue with this.

However, this program feels unreasonably small not to work

@jaustin
Copy link
Collaborator Author

jaustin commented Feb 9, 2023

And here's an example that should work totally fine on V1 unless the micro:bit experiences 6G

image

But this is still not going to fit on a V1 by 80 bytes

@microbit-carlos
Copy link
Collaborator

microbit-carlos commented Feb 9, 2023

For V1 we want to make sure the 927 panic code is only triggered at the point a V2-only feature is about to be executed, so a programme like the one Jonny posted above should run normally until the accelerometer 6G event is triggered.

Looking at the binary.js size numbers, it looks like the size difference between removing or including the [play sound] block is ~48 KBs. Ultimately this block for V1 is only going to execute the 927 panic, so it shouldn't be adding that much code to the V1 build.

  • Does this mean the [play sound] block is adding 48 KBs of flash data to the V1 builds as well?
  • Alternatively, does it mean the calculation to determine if the programme is too big is done with the V2 data instead of V1 data?

@abchatra
Copy link
Collaborator

abchatra commented Apr 6, 2023

@riknoll this should be fixed as well right?

@riknoll
Copy link
Member

riknoll commented Apr 6, 2023

Yup!

@microbit-carlos
Copy link
Collaborator

microbit-carlos commented Apr 6, 2023

Has the fix been deployed to beta already? Still seeing a large memory usage of the [play sound] block for V1, which should really only raise a panic.

@riknoll
Copy link
Member

riknoll commented Apr 6, 2023

no it has not, let me bump beta right now!

@microbit-carlos
Copy link
Collaborator

microbit-carlos commented Apr 6, 2023

Awesome, thanks!

Somewhat related, in beta at least, the original programme that Jonny posted has gotten worse in terms of memory. It used to be 252 bytes over the V1 limit and it is now 320 bytes over the limit. As DAL hasn't been updated, I assume the extra memory is consumed by MakeCode? Or is the size calculation made from the CODAL build?

image

@riknoll
Copy link
Member

riknoll commented Apr 6, 2023

shouldn't be based on the codal build... not sure what's causing the change there.

@microbit-carlos
Copy link
Collaborator

Just tested the latest 5.1.14 in beta and can confirm the V1 build is not as large anymore 🎉

Although removing the [play sound] block still has a significant difference in size.
Looking at binary.js with the ?compiler=size flag (don't know how to "download the hex file and open the mbcodal.asm file that appears under the built folder"), the difference there is around ~18KB:

With block:

// total=160711 new=4.74% cached=92.69% other=2.58%

Without it:

// total=142427 new=4.90% cached=92.19% other=2.90%

Which still feels like a lot. If this number is not accurate enough, how can we check the real number?

@riknoll
Copy link
Member

riknoll commented Apr 6, 2023

after you click the download button, the built folder in the file explorer below the sim (same place where you got binary.js) will contain two more files: mbdal.asm and mbcodal.asm. They only show up after you've done a download. Those have the actual binary compile size in comments at the top!

As for the other point, that's because we've implemented this in TypeScript instead of C++; we don't have IFDEFs in TS so the code is being included even in the micro:bit V1 compilations.

How much of a scenario is this for the micro:bit V1? if it's just going to panic anyway, do we expect people to be downloading programs for their V1 micro:bit that include these V2 blocks?

@abchatra
Copy link
Collaborator

abchatra commented Apr 6, 2023

I agree with Richard, lets not spend time on v1 size if the program is going to panic anyway.

@microbit-carlos
Copy link
Collaborator

I think this is still an important issue for micro:bit V2 users that will get a "programme too big" error because the V2 blocks are consuming more memory for V1 than they should.
While the beta or next MakeCode release would allow for downloading a V2 hex, that extra friction and confusion is not really necessary.

The original programme that Jonny posted now fits in beta, but just adding a couple of extra blocks hits the limit again:
image

I think we can agree that this is a relatively small programme (in number of blocks, and taking in consideration no extensions have been added) to be greeted by this message every time the user clicks "download".
image

@eanders-ms eanders-ms self-assigned this May 18, 2023
@jwunderl jwunderl removed their assignment May 22, 2023
@eanders-ms
Copy link
Collaborator

After the switch to gcc5, this program builds with 4k memory to spare. The improvement will be available in the v6 release (currently live in beta channel).

@microbit-carlos
Copy link
Collaborator

While this issue started a simple example to show how small programmes didn't fit in V1, I think we uncovered that some blocks that are V2-only seem to be taking large amounts of memory in V1 (>15KB), when they should only be triggering a panic in V1.

While this might not initially seem that important (after all, V2 blocks would fail in V1 anyway), the "out of V1 memory" error would would be shown all users and might be confusing.

@eanders-ms
Copy link
Collaborator

@microbit-carlos that makes sense. Can you please file an issue?

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

No branches or pull requests

6 participants