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

Unbox some types #1310

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Unbox some types #1310

merged 3 commits into from
Jan 12, 2024

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Dec 28, 2023

This unboxes some types:

  1. records with a single field (often used for second-order polymorphism reasons),
  2. variants with a single constructor with a single argument.

The idea is to optimize these by removing the extra layer of indirection. Moreover, it might be beneficial to avoid allocating all these Analyses.ask_of_ctx blocks.

Moreover, ask_of_ctx is eta-reduced to avoid additional closure allocation even with it unboxed.

TODO

  • Benchmark on sv-benchmarks.
  • Check Gobview compatibility.

@sim642 sim642 added the performance Analysis time, memory usage label Dec 28, 2023
@sim642 sim642 self-assigned this Dec 28, 2023
@michael-schwarz
Copy link
Member

Nice! I didn't know that OCaml already has this support for unboxed types in limited circumstances, I was only aware of the more general ideas and work on this at JaneStreet.
(e.g. https://github.com/ocaml/RFCs/blob/unboxed-types/rfcs/unboxed-types.md and https://icfp22.sigplan.org/details/mlfamilyworkshop-2022-papers/13/Unboxed-types-for-OCaml).

I was initially worried about what this means for our JavaScript build and marshaling, but given this feature has been there for a long time, I guess all the kinks should be ironed out by now.

@sim642
Copy link
Member Author

sim642 commented Dec 28, 2023

I was initially worried about what this means for our JavaScript build and marshaling, but given this feature has been there for a long time, I guess all the kinks should be ironed out by now.

Since these attributes are just for optimization, I suppose it'd be fine even if jsoo didn't optimize like this (although maybe it shares enough of the OCaml compilation pipeline to still do, not sure).
In such case the only potential is marshaled data, so I'll have to see if Gobview still works. Most of these shouldn't even be part of marshaled data, but the Inc thing in IntDomain might be.

@sim642
Copy link
Member Author

sim642 commented Jan 11, 2024

sv-benchmarks runs show ~4% CPU time improvement from these changes:
image

Most of it seems to come from be69a34, though. Or at least the benefit of unboxing only shows together with that change.
The reason being: unboxing removes one memory block indirection, eta-reduction removes a second one. As a result, ask_of_ctx is truly just a field lookup with no allocation.

@sim642 sim642 marked this pull request as ready for review January 11, 2024 15:21
@sim642 sim642 added this to the v2.4.0 milestone Jan 11, 2024
@sim642 sim642 merged commit eade39e into master Jan 12, 2024
15 of 17 checks passed
@sim642 sim642 deleted the unboxed branch January 12, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Analysis time, memory usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants