Skip to content

avoid creating unnecessary copies of allocating constants in field fl… #7421

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

Merged
merged 1 commit into from
May 5, 2025

Conversation

cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented May 5, 2025

…attening

Fixes #7406

The underlying issue of copying constants that allocate, such as empty record creation {}, seems to have always been there in the compiler.

fix: avoid creating unnecessary copies of allocating constants in field flattening

The field flattening optimization was creating unnecessary copies of allocating constants (like Const_block and Const_some), which can be inconsistent. This change makes the optimization only flatten fields when the constant doesn't allocate memory.

The original example in #7406 that assigns to a mutable field was losing the assignment to the mutable field entirely. This is probably because a copy of the nested object was made, then the assignment, and since the copy of the object is never exported, it might have been removed as dead code.

…attening

Fixes #7406

fix: avoid creating unnecessary copies of allocating constants in field flattening

The field flattening optimization was creating unnecessary copies of allocating constants
(like Const_block and Const_some), which could lead to memory issues. This change makes
the optimization only flatten fields when the constant doesn't allocate memory.
@cristianoc cristianoc requested review from cknitt and nojaf May 5, 2025 08:11
Copy link

pkg-pr-new bot commented May 5, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7421

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7421

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7421

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7421

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7421

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7421

commit: dec0cae

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix! 🎉

@cristianoc cristianoc merged commit 74b4273 into master May 5, 2025
21 checks passed
@cristianoc cristianoc deleted the field_flattening branch May 5, 2025 09:06
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.

V11 & V12 - Fails to set value to nested optional mutable field
2 participants