Share array entities if possible with ary.replace(frozen_ary)
#6576
Conversation
Currently mruby is limited to always placing array entities on the rewritable heap. Therefore, if the original array was frozen and only rewritable objects survived the subsequent process, the array entity can be changed. If an array object that actually shared the array entity is frozen with `ary.freeze`, there should be no problem, since the shared state is still kept.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @dearblue, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the ary.replace functionality in mruby to improve memory efficiency. It addresses a previous limitation where array entities from frozen source arrays could not be shared, leading to unnecessary memory allocations. By allowing sharing even with frozen arrays, the change aims to optimize memory usage during array replacement operations.
Highlights
- Array Sharing Logic: Modified the
ary_replacefunction insrc/array.cto remove the condition that prevented sharing array entities if the source array (b) was frozen. This change allowsary.replaceto share the underlying array entity of a frozen array, provided its length exceedsARY_REPLACE_SHARED_MIN.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to optimize Array#replace by allowing the underlying data of a frozen array to be shared. The change removes a check that prevented frozen arrays from being converted to shared arrays. However, this introduces a critical issue where the internal state of a frozen array is modified, violating Ruby's immutability contract for frozen objects. My review highlights this issue and asks for clarification on this fundamental change in behavior.
| return; | ||
| } | ||
| if (!mrb_frozen_p(b) && len > ARY_REPLACE_SHARED_MIN) { | ||
| if (len > ARY_REPLACE_SHARED_MIN) { |
There was a problem hiding this comment.
This change removes the !mrb_frozen_p(b) check, which allows ary_make_shared(mrb, b) to be called on a frozen array b.
The ary_make_shared() function modifies the internal state of the array passed to it. Specifically, it updates the aux union member to point to a mrb_shared_array struct and sets the MRB_ARY_SHARED flag:
// in ary_make_shared(mrb, a) where a is b
a->as.heap.aux.shared = shared;
ARY_SET_SHARED_FLAG(a);Modifying a frozen object, even its internal metadata, violates the immutability guarantee of frozen objects in Ruby. This could lead to unexpected behavior and subtle bugs if other parts of the system rely on frozen objects being truly immutable.
While mrb_obj_freeze() for arrays does shrink the capacity to match the length (avoiding a realloc inside ary_make_shared()), the modification of the RArray struct itself remains a serious concern.
Is this change in the semantics of frozen arrays intentional? If so, this is a significant change that should be carefully documented. Otherwise, this change appears to introduce a critical bug.
An alternative to achieve sharing from a frozen array would require a mechanism that does not modify the source frozen array, which would likely be a more significant change involving array and GC management.
There was a problem hiding this comment.
Is this change in the semantics of frozen arrays intentional? If so, this is a significant change that should be carefully documented. Otherwise, this change appears to introduce a critical bug.
If this is to be a future concern, I think an equivalent to RSTR_NOFREE_P() needs to be introduced at this time.
1 participant
Currently mruby is limited to always placing array entities on the rewritable heap. Therefore, if the original array was frozen and only rewritable objects survived the subsequent process, the array entity can be changed. If an array object that actually shared the array entity is frozen with
ary.freeze, there should be no problem, since the shared state is still kept.