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

[WIP] Apply &on-heap selectively #1781

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

[WIP] Apply &on-heap selectively #1781

wants to merge 5 commits into from

Conversation

rsmmr
Copy link
Member

@rsmmr rsmmr commented Jul 8, 2024

In our generated C++ parsing code, so far we generally moved all units
to the heap. This PR changes that to do so only for cases where we
actually need it (primarily: if we wrap the unit into a reference
somewhere, or if the unit type is cyclic). The assumption is that this
should improve both performance and memory usage.

  • Let optional's deref operator pass through "LHSideness".
  • Prepare for moving parser temporaries to the stack.
  • Add API to retrieve a type's dependent types.
  • Rework emitting of C++-side type declarations.
  • Apply &on-heap selectively only where needed.
  • Change a set to an unordered_set.

@rsmmr rsmmr changed the title Apply &on-heap selectively [WIP] Apply &on-heap selectively Aug 7, 2024
@rsmmr
Copy link
Member Author

rsmmr commented Aug 9, 2024

Rebased on top of #1833, now using our new AST dependency tracking.

Test suite still fails because of #1831.

The parser generator is creating temporaries at a few places that will
then later receive parsed values. Currently, those temporaries are
always simply of the type being parsed. Turns out that works for
units/structs only because we're always wrapping them into
`value_ref`, meaning we can create those temporaries as null values.
However, in a subsequent commit, we will get rid of those wrappings in
some cases, meaning the structs would need to be default initialized
at the time they are created on the stack. That doesn't work for some
struct types though: if a struct receives parameters, we would need to
pass them at instantiation time, but for these temporaries we don't
know parameters at that point. So to prepare for the upcoming  change,
this commit wraps such temporary struct values into `optional<>` iff
they receive parameters.

(I have been wondering if we should generally wrap all these parser
temporaries into optionals, independent of their type. Semantically
that would make sense, but I'm guessing the C++ compiler gets better
optimization opportunties if we don't.)
Previously, we would attach `&on-heap` to any `struct` compiled from a
Spicy `unit`. Now we only attach it only for specific cases that need it,
moving state to the stack otherwise.
For efficiency and consistency.
@rsmmr
Copy link
Member Author

rsmmr commented Aug 9, 2024

With #1833 and #1834 merged, this is now rebased on main.

Still not sure if we want this or not due to runtime performance concerns.

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.

1 participant