compiler,reflect: add support for [...]T -> []T in reflect

[dgryski]

No description provided.

[dgryski]

Pull in a commit from #3570

[aykevl]

Seems good to me (but see the comment below). Also, can you add a test, for example to testdata/reflect.go?

Unfortunately, this does increase binary size quite substantially (for a relatively rare feature): more than half of the driver smoke tests increased in size by some amount, and a number of them by >100 bytes. Overall it says 0.27% increase, which is quite a lot IMHO.

I suspect a significant contributor is all the unexported fields in src/device, some of which are arrays for padding (like [20]byte). Maybe a flag to not emit reflect information for unexported fields would help? I'm thinking -reflect=[full|minimal|none], where "minimal" could mean "no unexported fields".

[aykevl]

Does this work correctly with both small and large arrays? I'm thinking about valueFlagIndirect.

[dgryski]

Oh, good point. Let me add some more tests for that.

[dgryski]

While I was expecting some size increase, all the [20]byte for padding should only add a single entry pointing to a byte slice. My assumption was that anything use an array of things would also at some point use a slice of things, so the extra space would be literally the additional pointer in the array typeinfo (plus maybe some padding bytes for alignment).

[dgryski]

I wonder if that size increase (above the array->slice pointers) is the actual new reflect code that isn't getting stripped?

[aykevl]

My assumption was that anything use an array of things would also at some point use a slice of things

Unfortunately not. Reflect data is highly recursive, so using a PWM interface in a driver is enough to keep data for machine.PWM, which refers to data for nrf.PWM_Type, which contains a number of padding arrays of varying sizes. Even when no reflect features are used and the struct fields themselves are entirely useless (they're _).

[deadprogram]

I'm thinking -reflect=[full|minimal|none], where "minimal" could mean "no unexported fields".

So -reflect=none means do not include support for reflection?

For embedded code, minimal could be the default? I wonder if the compiler could detect the needed level of reflection support.

Finally wouldn't adding a flag like this be a feature to add in a separate PR?

[aykevl]

So -reflect=none means do not include support for reflection?

Yeah. But that also means removing some other language features like hashmaps with arbitrary keys, so maybe it's not such a great idea.

For embedded code, minimal could be the default?

That's certainly something we could consider, as long as it doesn't affect too much existing code.

I wonder if the compiler could detect the needed level of reflection support.

Sadly not (except for trivial cases which are probably already optimized). You quickly run into halting-problem level of difficulty.

Finally wouldn't adding a flag like this be a feature to add in a separate PR?

Oh definitely, this is not something that should be part of this PR.

[aykevl]

I wonder if that size increase (above the array->slice pointers) is the actual new reflect code that isn't getting stripped?

I did a quick test with tinygo build -size full -o ./build/test.hex -target=circuitplay-bluefruit ./examples/tone and no, it really is the reflect data (124 bytes). The entire firmware size difference is in the "Go types" pseudo-package.

[deadprogram]

So we are waiting for some additional tests and then we will merge this PR?

[dgryski]

Seems like with the size increase we probably want to leave this for now.

[soypat]

For embedded code, minimal could be the default?

As a long time user of TinyGo for embedded systems for hobby and just recently professional use I'd like to chime in and say this change would not affect me much since I'm almost entirely using the Raspberry Pi Pico (0.25% increase is a measly 30 bytes diff of the 264kB available). Surely there's other targets where a 30 or 100bytes increase is noticeable or even mission critical, but I don't think it's the case for Pico users which would warrant -target=pico to compile with -reflect=minimal by default.

[deadprogram]

What code that does not currently compile does this PR unlock for us? That would make it easier to help decide if the cost in bytes is worth it.

[aykevl]

I'm careful with things like "it's only 0.25%" because binaries in the 0.28.0 release caused a ~10% increase in binary size, and I think the majority of that is these small seemingly insignificant binary size increases. In embedded systems, that could just be what's pushing the firmware size over the edge and make it too big for a flash chip.

What code that does not currently compile does this PR unlock for us? That would make it easier to help decide if the cost in bytes is worth it.

This would be very helpful indeed.

[dgryski]

I think I found this trying to make the encoding/binary tests pass.

[deadprogram]

These are the top 5 size increased in the smoketests from this PR:

10284   10408    124   1.21%  tinygo build -size short -o ./build/test.hex -target=circuitplay-bluefruit ./examples/tone
6420    6484     64   1.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/ssd1306/i2c_128x32/main.go
11780   11896    116   0.98%  tinygo build -size short -o ./build/test.hex -target=nano-33-ble ./examples/apds9960/proximity/main.go
5124    5168     44   0.86%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/pcd8544/setpixel/main.go
7920    7984     64   0.81%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/at24cx/main.go

Make of the smoketests did not change size at all. I wonder why these changed by the largest percentage?

[dkegel-fastly]

Suggest releasing without this feature, then landing it right after release.

Time marches on.
https://www.microchip.com/en-us/product/ATmega328P (32KB flash) is "not recommended for new designs". Its replacement, https://www.microchip.com/en-us/product/atmega328pb, has 64KB flash.
Folks who need to save that last iota of flash space could try the last release before this feature lands.
But in general, I suspect each additional standard library test suite that passes increases the audience for tinygo enough to be worth an iota or two.

[deadprogram]

Suggest releasing without this feature, then landing it right after release.

@dkegel-fastly that is very sensible advice. Moving milestone.

[deadprogram]

The time has come to merge this PR and unlock the additional refection compatibility it brings.

Thank you very much for all your work on this feature @dgryski and to everyone else for your helpful comments and feedback.