Skip to content

compiler,reflect: support making **T, [...]T -> []T#3570

Closed
dgryski wants to merge 4 commits into
tinygo-org:devfrom
dgryski:dgryski/reflect-make-starstarT
Closed

compiler,reflect: support making **T, [...]T -> []T#3570
dgryski wants to merge 4 commits into
tinygo-org:devfrom
dgryski:dgryski/reflect-make-starstarT

Conversation

@dgryski

@dgryski dgryski commented Mar 17, 2023

Copy link
Copy Markdown
Member

Hacky solution. Uses a bunch of extra space for things that probably won't be needed. Should probably be hidden behind a compiler option until we something better. Marking as draft. Putting here so I don't lose it.

@dgryski dgryski force-pushed the dgryski/reflect-make-starstarT branch 2 times, most recently from 15dadcc to e33d7d1 Compare March 20, 2023 20:20
@dgryski dgryski changed the title DO NOT MERGE: compiler,reflect: support making **T DO NOT MERGE: compiler,reflect: support making **T, [...]T -> []T Mar 20, 2023
@dgryski

dgryski commented Mar 21, 2023

Copy link
Copy Markdown
Member Author

I think this second commit shouldn't be that space expensive since almost every time there's an array of a type there will already be a slice of that type.

@dgryski

dgryski commented Mar 24, 2023

Copy link
Copy Markdown
Member Author

I still think this should be merged even if we put it behind a compiler flag. Any kind of deserialization will need this capability. We can try to shrink it later, but until we have runtime type lookup/creation, I think this is a reasonable approach.

@dgryski dgryski marked this pull request as ready for review March 25, 2023 18:01
@dgryski

dgryski commented Mar 25, 2023

Copy link
Copy Markdown
Member Author

Will fix the merge conflicts, but this is required for any kind of reflection deserialization.

@dgryski dgryski force-pushed the dgryski/reflect-make-starstarT branch from e33d7d1 to b22eadb Compare March 26, 2023 22:41
@dgryski dgryski changed the title DO NOT MERGE: compiler,reflect: support making **T, [...]T -> []T compiler,reflect: support making **T, [...]T -> []T Mar 26, 2023
@dgryski dgryski force-pushed the dgryski/reflect-make-starstarT branch from c9aa44d to d1b62a4 Compare March 27, 2023 17:35
@deadprogram deadprogram requested a review from aykevl March 28, 2023 16:30
@aykevl

aykevl commented Mar 29, 2023

Copy link
Copy Markdown
Member

I don't think we should be doing this, as it's a significant size regression: it adds around 2% of binary size. That is sometimes multiple kilobytes of extra flash area. With all the previous patches, the size regression was generally much more manageable.

A better way would be to work on creating types at runtime.

@dgryski

dgryski commented Mar 29, 2023

Copy link
Copy Markdown
Member Author

@aykevl Do you have an idea for how to proceed with that? If not, could we merge this behind a compiler option and remove it in the future when a better design appears?

@dgryski

dgryski commented Mar 30, 2023

Copy link
Copy Markdown
Member Author

I'll try to come up with something.

@dgryski

dgryski commented Mar 30, 2023

Copy link
Copy Markdown
Member Author

So I'm thinking additional side tables consisting of type pointers. And if what we want isn't there we add it to a global map. I have a bunch of stuff on my plate at the moment but I'll try to get a prototype working.

@dgryski

dgryski commented Mar 30, 2023

Copy link
Copy Markdown
Member Author

I'm going to work on a side table for the pointer-to-pointer case; I can't imagine very many programs have an array without the equivalent slice type, so I don't think that significantly increasing the space usage.

@dgryski dgryski force-pushed the dgryski/reflect-make-starstarT branch from d1b62a4 to 50615b6 Compare March 31, 2023 15:28
@deadprogram deadprogram added this to the v0.28.0 milestone Apr 23, 2023
@deadprogram

Copy link
Copy Markdown
Member

@dgryski can you please resolve merge conflict?

@aykevl

aykevl commented Apr 27, 2023

Copy link
Copy Markdown
Member

I still don't think we should merge this PR as-is because it results in such a size regression.

I have a possible solution in mind but it's going to need some work. Namely by not storing "pointer to" types at all but instead storing them in the lower bits of the type pointer (while ensuring type structs are always aligned).
In more detail:

  • Remove the ptrTo field, it won't be necessary anymore.
  • Make sure all type structs are 32-bit aligned. This means the lower two bits of the pointer are zero.
  • A type *T can then be represented by adding one to the pointer (this is equivalent to setting the lower bit to one).
  • A type **T can be represented by adding two to the pointer.
  • Same for ***T. A type ****T cannot be represented this way, we may still need a pointer type.
  • Kind() returns PointerType when the lower two bits aren't zero, or else it behaves as it already does.
  • Elem() subtracts one from the pointer if it is a pointer type, else it works as it already does.

(This is the system I mentioned in the Slack channel a while ago, but never got working completely).

@dgryski what do you think? Is this something you'd like to work on or should I continue my (very incomplete) work in this direction?

@dgryski

dgryski commented Apr 27, 2023

Copy link
Copy Markdown
Member Author

I like this idea of pointer tagging. In all of my testing I don't think I've seen a need for ***T so having that as an upper limit seems totally reasonable. I have time to help work on this, if needed.

@dgryski

dgryski commented Apr 27, 2023

Copy link
Copy Markdown
Member Author

Filed #3684 to track this new implementation.

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.

3 participants