bug(#4584): resize on writing to malloc#4994
Conversation
📝 WalkthroughWalkthroughThis pull request modifies malloc memory management to support automatic block resizing instead of throwing errors on out-of-bounds writes. Test cases in EO and Java are updated to verify successful resizing behavior, and the core Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
eo-runtime/src/main/eo/malloc.eo (1)
293-300: Naming: use thetests-prefix for positive tests.All other success-path tests in this file are named
tests-…, andthrows-on-…is reserved for tests that expect a failure. Since these two are now success-path assertions (replacing formerthrows-on-…tests), renaming totests-writes-beyond-offset-and-resizesandtests-copies-and-resizes-to-targetwould keep the convention consistent and make their intent obvious at a glance.Also applies to: 385-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/main/eo/malloc.eo` around lines 293 - 300, Rename the positive test identifiers to follow the project convention: change the test name writes-beyond-offset-and-resizes to tests-writes-beyond-offset-and-resizes (in the malloc.of test that contains m.write and m.size.eq) and likewise change the other positive test (the one around the copy-and-resize behavior referenced at lines 385-392) from its current non-prefixed name to tests-copies-and-resizes-to-target so that all success-path tests use the tests-… prefix and failure tests keep throws-on-…; update only the test names (identifiers) where they are declared.eo-runtime/src/test/java/org/eolang/HeapsTest.java (1)
126-135: Consider strengthening this test to cover leading bytes and final size.The assertion only reads back the three bytes that were just written; a regression that clobbered the leading byte at offset 0 or grew the block past
offset + data.lengthwould not be caught. Reading from offset 0 for 4 bytes (expecting{0, 1, 2, 3}) or assertingHeaps.INSTANCE.size(idx) == 4would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/test/java/org/eolang/HeapsTest.java` around lines 126 - 135, Update the test resizesOnWriteMoreThanAllocatedWithOffset to also verify leading byte and final block size: after calling Heaps.INSTANCE.write(idx, 1, new byte[] {1,2,3}), read from offset 0 for 4 bytes via Heaps.INSTANCE.read(idx, 0, 4) and assert the result equals {0,1,2,3}, and also assert Heaps.INSTANCE.size(idx) == 4 to ensure the block resized exactly to offset+data.length; keep the existing free(idx) cleanup.eo-runtime/src/main/eo/io/malloc-as-output.eo (1)
63-72: Consider prefixing the test name withtests-for consistency.Other positive tests in this file and in
malloc.eouse thetests-prefix (tests-makes-an-output-from-malloc-and-writesright above), whilethrows-on-is reserved for negative expectations. Since this is now a success-path assertion,tests-writes-larger-data-than-provided-blockwould align with the existing convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/main/eo/io/malloc-as-output.eo` around lines 63 - 72, Rename the test label "writes-larger-data-than-provided-block" to follow the project's positive-test convention by prefixing it with "tests-"; update the test declaration in the malloc-as-output.eo diff so the label becomes "tests-writes-larger-data-than-provided-block" (the test that calls malloc.of and (malloc-as-output mem).write 01-02-03 should be the one renamed) to match other positive tests like "tests-makes-an-output-from-malloc-and-writes".eo-runtime/src/main/java/org/eolang/Heaps.java (1)
180-188: Redundant allocation and copy when the block is resized.When the resize branch is taken,
this.resize(...)already allocates a new array of sizeoffset + data.lengthand copies the old contents into it. Immediately after, lines 183–186 allocate yet another array of the same length and copy the same contents again, so you pay for two full-size allocations plus copies on every resizing write. The non-resize path also unnecessarily clones the existing buffer — sinceread()already returnsArrays.copyOfRange(...), readers never observe the stored array, so an in-place write under the lock is safe.Consider inlining the growth and writing directly into the stored buffer:
♻️ Proposed refactor
- if (this.blocks.get(identifier).length < offset + data.length) { - this.resize(identifier, offset + data.length); - } - final byte[] source = this.blocks.get(identifier); - final int length = source.length; - final byte[] result = new byte[length]; - System.arraycopy(source, 0, result, 0, length); - System.arraycopy(data, 0, result, offset, data.length); - this.blocks.put(identifier, result); + byte[] bytes = this.blocks.get(identifier); + if (bytes.length < offset + data.length) { + final byte[] resized = new byte[offset + data.length]; + System.arraycopy(bytes, 0, resized, 0, bytes.length); + bytes = resized; + this.blocks.put(identifier, bytes); + } + System.arraycopy(data, 0, bytes, offset, data.length);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/main/java/org/eolang/Heaps.java` around lines 180 - 188, The current write logic performs an extra allocation+copy: after calling this.resize(identifier, ...) you immediately clone the buffer again and copy data into the clone; also the non-resize path clones the buffer unnecessarily. Fix by obtaining the stored byte[] once (this.blocks.get(identifier)) after any resize and writing data directly into that array at offset (System.arraycopy(data, 0, buf, offset, data.length)); do the same in the non-resize path—avoid creating the intermediate result array. Keep using the lock and note that read() already returns a copy, so in-place writes to this.blocks are safe; update code around Heaps.resize, this.blocks, identifier, data, and offset accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@eo-runtime/src/main/eo/io/malloc-as-output.eo`:
- Around line 63-72: Rename the test label
"writes-larger-data-than-provided-block" to follow the project's positive-test
convention by prefixing it with "tests-"; update the test declaration in the
malloc-as-output.eo diff so the label becomes
"tests-writes-larger-data-than-provided-block" (the test that calls malloc.of
and (malloc-as-output mem).write 01-02-03 should be the one renamed) to match
other positive tests like "tests-makes-an-output-from-malloc-and-writes".
In `@eo-runtime/src/main/eo/malloc.eo`:
- Around line 293-300: Rename the positive test identifiers to follow the
project convention: change the test name writes-beyond-offset-and-resizes to
tests-writes-beyond-offset-and-resizes (in the malloc.of test that contains
m.write and m.size.eq) and likewise change the other positive test (the one
around the copy-and-resize behavior referenced at lines 385-392) from its
current non-prefixed name to tests-copies-and-resizes-to-target so that all
success-path tests use the tests-… prefix and failure tests keep throws-on-…;
update only the test names (identifiers) where they are declared.
In `@eo-runtime/src/main/java/org/eolang/Heaps.java`:
- Around line 180-188: The current write logic performs an extra
allocation+copy: after calling this.resize(identifier, ...) you immediately
clone the buffer again and copy data into the clone; also the non-resize path
clones the buffer unnecessarily. Fix by obtaining the stored byte[] once
(this.blocks.get(identifier)) after any resize and writing data directly into
that array at offset (System.arraycopy(data, 0, buf, offset, data.length)); do
the same in the non-resize path—avoid creating the intermediate result array.
Keep using the lock and note that read() already returns a copy, so in-place
writes to this.blocks are safe; update code around Heaps.resize, this.blocks,
identifier, data, and offset accordingly.
In `@eo-runtime/src/test/java/org/eolang/HeapsTest.java`:
- Around line 126-135: Update the test resizesOnWriteMoreThanAllocatedWithOffset
to also verify leading byte and final block size: after calling
Heaps.INSTANCE.write(idx, 1, new byte[] {1,2,3}), read from offset 0 for 4 bytes
via Heaps.INSTANCE.read(idx, 0, 4) and assert the result equals {0,1,2,3}, and
also assert Heaps.INSTANCE.size(idx) == 4 to ensure the block resized exactly to
offset+data.length; keep the existing free(idx) cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75b232fb-672a-4a28-a9ce-d360c884bd98
📒 Files selected for processing (4)
eo-runtime/src/main/eo/io/malloc-as-output.eoeo-runtime/src/main/eo/malloc.eoeo-runtime/src/main/java/org/eolang/Heaps.javaeo-runtime/src/test/java/org/eolang/HeapsTest.java
|
@yegor256 please check |
|
@maxonfjvipon Great work on your contribution! 🎉 You've earned +16 points and your total score is now +76. Keep up the excellent pace and don't forget to check your Zerocracy account for updates! |
3 participants
Closes: #4584
Summary by CodeRabbit
Release Notes