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

Templated arrayshrinkfit #21152

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

solo-source
Copy link
Contributor

This PR is for demonstrating the protoype implementation of the _d_arrayshrinkfit runtime hook.
This implementation:

  • Creates a templated version of the hook: _d_arrayshrinkfitT in core/internal/array/capacity.d
  • Maintains compatibility by forwarding calls to the original implementation
  • Adds a basic unittest to verify the functionality
  • Preserves handling of D_TypeInfo version conditions

Implementation Details
The implementation:

  1. Adds a new templated function _d_arrayshrinkfitT that forwards to the original implementation
  2. Updates object.d to use this templated version in the assumeSafeAppend function
  3. Maintains proper handling for D_TypeInfo version conditions
  4. Adds a unittest to verify the basic functionality

Testing
I've added a simple unittest to verify the basic functionality. The test creates an array with extra capacity, applies the shrinkfit function, and checks that the array contents remain unchanged. While running the full druntime-test suite shows some failures in the profile subsystem, these appear unrelated to this change. I have added a screehshot capturing the error below.
ScreenshotArrayShrinkFit

I have a few questions

  • Is the forwarding approach correct?
  • For full templated implementation, should I start by directly replacing the TypeInfo usage, or should I take a more gradual approach?

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @solo-source! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#21152"

Copy link
Member

@teodutu teodutu left a comment

Choose a reason for hiding this comment

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

I made an initial review. This is a good starting point. Now to start templating the hook itself, first make sure all tests pass with this approach. Then copy the logic of the old one in the template and extract the TypeInfo using typeid. When all tests pass with this approach, start replacing TypeInfo usage with the template T and static ifs.

The current test failure still has to do with your hook [1].

[1] https://github.com/dlang/dmd/actions/runs/14283753130/job/40036353251?pr=21152

@solo-source
Copy link
Contributor Author

solo-source commented Apr 7, 2025

Hi @teodutu i tried to isolate and reproduce the failing behavior i was observing on my local machine at

assert(a.ptr == c.ptr);
using a minimal reproduction listed below.

import std.stdio;

@system unittest
{
    int[] a = [1, 2, 3, 4];

    // Without assumeSafeAppend. Appending relocates.
    int[] b = a[0 .. 3];
    writeln("Case 1: Without assumeSafeAppend");
    writeln("  b.ptr before appending: ", b.ptr);
    b ~= 5;
    writeln("  a.ptr: ", a.ptr, "  b.ptr after appending: ", b.ptr);
    assert(a.ptr != b.ptr, "Expected b.ptr to differ from a.ptr after reallocation");

    // With assumeSafeAppend. Appending should not relocate.
    int[] c = a[0 .. 3];
    writeln("\nCase 2: With assumeSafeAppend");
    writeln("  c.ptr before assumeSafeAppend: ", c.ptr);
    c.assumeSafeAppend() ~= 5;
    writeln("  a.ptr: ", a.ptr, "  c.ptr after assumeSafeAppend: ", c.ptr);
    assert(a.ptr == c.ptr, "Expected c.ptr to remain equal to a.ptr");
}

void main() {} // Dummy main to allow standalone compilation

While executing this i am getting the following Output

C:\Source\D\dmd>C:\Source\D\dmd\generated\windows\release\64\dmd.exe "C:\Users\SamrendraSingh\Desktop\D Files\test_assumeSafeAppend.d" -unittest -debug -of=test_assumeSafeAppend.exe

C:\Source\D\dmd>"C:\Users\SamrendraSingh\Desktop\D Files\test_assumeSafeAppend.exe"
Case 1: Without assumeSafeAppend
  b.ptr before appending: 272DB0A0020
  a.ptr: 272DB0A0020  b.ptr after appending: 272DB0A1000

Case 2: With assumeSafeAppend
  c.ptr before assumeSafeAppend: 272DB0A0020
  a.ptr: 272DB0A0020  c.ptr after assumeSafeAppend: 272DB0A1030

core.exception.AssertError@C:\Users\SamrendraSingh\Desktop\D Files\test_assumeSafeAppend.d(21): Expected c.ptr to remain equal to a.ptr
----------------
0x00007FF6CC487737
0x00007FF6CC487090
0x00007FF6CC4713F2
0x00007FF6CC486E6D
0x00007FF6CC49A87E
0x00007FF6CC4997A7
0x00007FF6CC49EB84
0x00007FF6CC49C18C
0x00007FF6CC49EB0B
0x00007FF6CC499777
0x00007FF6CC49A720
0x00007FF6CC48D0A5
0x00007FF6CC48D01F
0x00007FF6CC48CF2A
0x00007FF6CC487299
0x00007FF6CC471424
0x00007FF6CC4C90D0
0x00007FFB81A6259D in BaseThreadInitThunk
0x00007FFB82EAAF38 in RtlUserThreadStart
1/1 modules FAILED unittests

C:\Source\D\dmd>

I am stumped, can’t pin point why this is happening, what are your thoughts on this?

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.

3 participants