Skip to content

Translate _d_array{,set}ctor to templates#13116

Merged
RazvanN7 merged 8 commits intodlang:masterfrom
teodutu:convert-_d_arrayctor-hooks
Dec 4, 2021
Merged

Translate _d_array{,set}ctor to templates#13116
RazvanN7 merged 8 commits intodlang:masterfrom
teodutu:convert-_d_arrayctor-hooks

Conversation

@teodutu
Copy link
Copy Markdown
Member

@teodutu teodutu commented Sep 30, 2021

This PR is part of replacing runtime hooks in DMD with templates, as described in this project.
I also provided a short description of this project in this forum post.

This PR concludes Dan Printzell's previous work:

@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request and interest in making D better, @teodutu! 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#13116"

@WalterBright
Copy link
Copy Markdown
Member

Missing is why do this. What problem is being solved. It adds a bunch of code.

@PetarKirov
Copy link
Copy Markdown
Member

@WalterBright You can find more information here: https://forum.dlang.org/thread/bbhmgddyabnuxajlxgqp@forum.dlang.org

@teodutu can you please update the commit message and pull request description with a brief information about the project goals and why the change is necessary?

@teodutu
Copy link
Copy Markdown
Member Author

teodutu commented Oct 5, 2021

@teodutu can you please update the commit message and pull request description with a brief information about the project goals and why the change is necessary?

I mentioned the initial proposal: dlang/project-ideas#25
I hope this provides enough context.

@teodutu
Copy link
Copy Markdown
Member Author

teodutu commented Oct 7, 2021

The code should pass (more) tests when this PR is merged: dlang/druntime#3582

@RazvanN7
Copy link
Copy Markdown
Contributor

RazvanN7 commented Oct 8, 2021

@teodutu Rebase this and lets see what happens.

@RazvanN7
Copy link
Copy Markdown
Contributor

RazvanN7 commented Oct 8, 2021

src/dmd/expressionsem.d' contains trailing whitespace at line 9763

@teodutu teodutu force-pushed the convert-_d_arrayctor-hooks branch from 12bc7f5 to 7cc4aec Compare October 8, 2021 15:37
@teodutu
Copy link
Copy Markdown
Member Author

teodutu commented Oct 9, 2021

This is currently blocked by this issue: https://issues.dlang.org/show_bug.cgi?id=22372

Vild and others added 7 commits December 2, 2021 22:25
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
@teodutu teodutu force-pushed the convert-_d_arrayctor-hooks branch from 9ff61bb to 44221d8 Compare December 2, 2021 20:37
@teodutu
Copy link
Copy Markdown
Member Author

teodutu commented Dec 2, 2021

In this post, I highlighted that adding a third parameter to _d_arrayctor yielded better performance than declaring the returned array inside the hook. For this reason, I chose to go back to the former approach, which is implemented in druntime here: dlang/druntime#3587.

@teodutu teodutu force-pushed the convert-_d_arrayctor-hooks branch 2 times, most recently from f3847b4 to 7e532af Compare December 2, 2021 22:11
druntime PR: dlang/druntime#3587

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
@teodutu teodutu force-pushed the convert-_d_arrayctor-hooks branch from 7e532af to eeb7f7f Compare December 3, 2021 19:12
@RazvanN7 RazvanN7 merged commit 71839d3 into dlang:master Dec 4, 2021
@RazvanN7
Copy link
Copy Markdown
Contributor

RazvanN7 commented Dec 4, 2021

I'm so happy this finally got in. Congrats for this awesome work @teodutu !

@andralex
Copy link
Copy Markdown
Member

andralex commented Dec 4, 2021

Awesome work, thanks! I just took a look at https://github.com/dlang/druntime/pull/2655/files and there are a few opportunities for improvements:

  • Instead of doing the memcpy element + postblit loop, we can do just one big memcpy for the whole thing for certain (many!) types. Introspection will figure which is which.
  • Types that don't throw from ctors (information we ca n get by introspection) don't need the try/catch. So we're looking at routines tailored for categories of types.
  • Could some or all of the work of enforceRawArraysConformable be done during compilation?
  • auto element_size = T.sizeof; is at best redundant and at worse a pessimization. Just use T.sizeof in the call to memcpy to make sure the compiler sees it's a compile-time constant.

@radcapricorn
Copy link
Copy Markdown
Contributor

radcapricorn commented Dec 4, 2021

To add to Andrei's notes, if these functions are going to be instantiated by user code, the try/catches also need a version(D_BetterC) version.

@teodutu teodutu deleted the convert-_d_arrayctor-hooks branch December 6, 2021 13:01
teodutu added a commit to teodutu/druntime that referenced this pull request May 26, 2022
These hooks have been converted to templates and their lowerings changed
to use those templates:
- dlang/dmd#13116
- dlang/dmd#13495
- dlang/dmd#13398

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
teodutu added a commit to teodutu/druntime that referenced this pull request May 26, 2022
These hooks have been converted to templates and their lowerings changed
to use those templates:
- dlang/dmd#13116
- dlang/dmd#13495
- dlang/dmd#13398

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
thewilsonator pushed a commit to dlang/druntime that referenced this pull request May 27, 2022
These hooks have been converted to templates and their lowerings changed
to use those templates:
- dlang/dmd#13116
- dlang/dmd#13495
- dlang/dmd#13398

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
@ibuclaw
Copy link
Copy Markdown
Member

ibuclaw commented Jun 13, 2022

This PR caused a regression. https://issues.dlang.org/show_bug.cgi?id=23181

kinke pushed a commit to ldc-developers/ldc that referenced this pull request Nov 25, 2022
These hooks have been converted to templates and their lowerings changed
to use those templates:
- dlang/dmd#13116
- dlang/dmd#13495
- dlang/dmd#13398

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review:student Symmetry autumn of code/Google Summer of Code

Projects

None yet

Development

Successfully merging this pull request may close these issues.