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

More stack-efficient struct DUs #1311

Open
6 tasks done
gimbling-away opened this issue Sep 1, 2023 · 13 comments
Open
6 tasks done

More stack-efficient struct DUs #1311

gimbling-away opened this issue Sep 1, 2023 · 13 comments

Comments

@gimbling-away
Copy link

I propose we use C-style unions for more stack-efficient struct DUs.

The existing way of approaching this problem in F# is that every variant is its own field, which unnecessarily uses stack space.

Pros and Cons

The advantages of making this adjustment to F# are struct DUs now use much less stack space.

The disadvantages of making this adjustment to F# are None, afaik. 😄

Extra information

Estimated cost (XS, S, M, L, XL, XXL): M (I would love to contribute this into FSC ^^)

Related suggestions: Can't find any.

Affidavit (please submit!)

Please tick these items by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on StackOverflow) and I have searched StackOverflow for discussions of this issue
  • This is a language change and not purely a tooling change (e.g. compiler bug, editor support, warning/error messages, new warning, non-breaking optimisation) belonging to the compiler and tooling repository
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

What could this translate into?

I tried to implement this in user code, and got pretty far!

https://gist.github.com/gimbles/2f005bf1e02176fa980e111bbb4f8f7d

My stack-efficient DU only takes 16 bytes on the stack, while the normal one takes 40!
The space taken by the DU will only be sizeof<tag> + sizeof<largest variant>.

@gimbling-away
Copy link
Author

Ah, this has already been suggested before. I tried searching for it, couldn't find it earlier. Closing for #699

@gimbling-away gimbling-away closed this as not planned Won't fix, can't repro, duplicate, stale Sep 1, 2023
@Happypig375
Copy link
Contributor

@gimbles In the Discord server you wrote that issue is only about "same type" right? Meanwhile you want to optimise for the case of reference type overlap with value type as well? This should be more than that issue.

@gimbling-away
Copy link
Author

@Happypig375 While #699 didn't focus on this specifically, this was addressed in the thread a few messages down.

@jl0pd
Copy link

jl0pd commented Sep 2, 2023

It's not possible to create type where field for reference type overlaps with field for value type.

Offset values shall be non-negative. It is possible to overlap fields in this way, though offsets occupied by an object reference shall not overlap with offsets occupied by a built-in value type or a part of another object reference. While one object reference can completely overlap another, this is unverifiable

(C) Ecma-335 II.10.7

Example: https://sharplab.io/#v2:DYLglsA0AuIE4FcB2AfAAgNgAQEECwAUAN6EC+hhAdAMbACGAzg1gG53AICmWnAHgA7Aw1MNCwBZAJ4BlaImrRCJAllVZKAMzCdgAEywBtAAwBdLGCTQAzACYsASUsAxbXsJr1WnfuNmGciwBzLFk4IJdvMgoCNAB2LABhdzU0AA4lZNU0AE4sfzpoYUyPVgB7MH1xLAAKAEosNCti5RKUgBZijzQAVix0gk61SmBS6nZmC1EaqVCEBVro1tU9EbGsI0HVSehSgCMAKwkZOTnFAZUluE4zj3ICUiA===

@gimbling-away
Copy link
Author

Yes, this is correct. This can be circumvented by using a GCHandle. Look at the gist I linked.

@T-Gro
Copy link

T-Gro commented Sep 4, 2023

1 comment on the gist linked:

While this works for a selected example, it is hard to imagine how this would work universally for any kind of DU.
Since DUs can have multiple fields, and those can be structs as well, proper calculation of the offsets is needed.

The catch is that sizeof is a runtime concern, not a compile-time one (size of native types, padding used etc.). Whereas the attributes have to be specified in the generated code already.

@gimbling-away
Copy link
Author

Wdym? The offset would be the exact same for all types, irrelevant to whether it's a struct or reference. For all variants, the offset would be 2 (leaving the tag behind).

@T-Gro
Copy link

T-Gro commented Sep 5, 2023

If there is a just one field in a case, yes.
As soon as there are 2+, you have to lay them out after each other and therefore need to know how much space each field (a primitive, a nested struct,..) takes.

@Tarmil
Copy link

Tarmil commented Sep 5, 2023

I suppose you could store multiple fields as a single struct tuple though, which would let the runtime lay it out as needed.

@T-Gro
Copy link

T-Gro commented Sep 5, 2023

Yes, that would work memory-layout-wise, in exchange for the small addition of 1 more indirection.
But indeed, this way it would be workable even without introducing new support for it in the runtime.

@gimbling-away
Copy link
Author

There is still a blocker for this from the runtime, unrelated to this. Generic types can not have custom struct layout. As mentioned in #699.

@NinoFloris
Copy link

Additionally there are pessimizations in the JIT around enregistration when a struct has an explicit layout.

@gimbling-away
Copy link
Author

This does come with an additional cost of having to implement IDisposable, this naturally can not become a default. However, this is still possible with an attribute, which would make it a non-breaking change.

As suggested here earlier to split the suggestions into different issues. This being for C/++ style, #699 for merging same-type. Re-opening.

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

No branches or pull requests

6 participants