Refactor _d_arraysetlengthT and _d_arraysetlengthiT to a single Template-Based Implementation#21089
Refactor _d_arraysetlengthT and _d_arraysetlengthiT to a single Template-Based Implementation#21089shivangshukla7020 wants to merge 0 commit intodlang:masterfrom
Conversation
|
Thanks for your pull request and interest in making D better, @shivangshukla7020! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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 referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#21089" |
|
Fixing the pre-commit checks |
teodutu
left a comment
teodutu
left a comment
There was a problem hiding this comment.
I made some initial comments. As a general note, you'll have an easier time reusing as much of the old code as you can (everything except TypeInfo) and after all tests pass you can try to make improvements. Don't reinvent the wheel too early.
compiler/src/dmd/expressionsem.d
Outdated
| Type tn = ale.e1.type.toBasetype().nextOf(); | ||
| checkDefCtor(ale.loc, tn); | ||
|
|
||
| // Choose correct GC hook |
There was a problem hiding this comment.
| // Choose correct GC hook | |
| // Choose the correct hook |
Technically only _d_arraysetlengthTTrace is a "GC hook" and all it does extra is that it prints some statistics about allocations. This one is used when compiling with the -profile=gc argument.
| * Fully templated implementation of `_d_arraysetlengthT`, removing reliance on `TypeInfo`. | ||
| * The old `_d_arraysetlengthT` and `_d_arraysetlengthiT` functions have been removed. | ||
| */ |
There was a problem hiding this comment.
No need to reference the old hooks.
| * This function is **now fully templated**, eliminating the need for TypeInfo-based | ||
| * `_d_arraysetlengthT` and `_d_arraysetlengthiT`, while efficiently handling both | ||
| * zero-initialized and custom-initialized types. |
There was a problem hiding this comment.
Again, no need to reference the old hooks. Just copy and paste the ddoc comments from rt/lifetime.d and update the parameters.
| template _d_arraysetlengthTImpl(Tarr : T[], T) | ||
| /// Complete templated implementation of `_d_arraysetlengthT` and its GC profiling variant `_d_arraysetlengthTTrace` | ||
|
|
||
| size_t _d_arraysetlengthT(Tarr : T[], T)( |
There was a problem hiding this comment.
Consider updating the lowering to use this signature:
size_t _d_arraysetlengthT(Tarr : T[], T)(return scope ref Tarr arr, size_t newlength, bool isMutable)Then the compiler should unqualify arr itself before calling the hook in expressionsem.d like so [1]. The point of this would be to only emit one template instance regardless of type qualifiers. So instead of using is (T = ...) below you'd just use isMutable). This might not work, so experiment with it first. It's best to apply this suggestion last, when everything else works.
[1]
dmd/compiler/src/dmd/expressionsem.d
Lines 5425 to 5432 in 8c4a95f
There was a problem hiding this comment.
The diffference between _d_arraysetlenghtT and _d_arraysetlengthiT is that the former initialises all elements to 0 (so a simple memset would do it) while the latter uses the .init symbol, like you did. You should create a distinction between the two cases somewhere. Is this taken into account and I missed it?
| return 0; | ||
| } | ||
|
|
||
| static if (is(T == immutable) || is(T == const)) |
There was a problem hiding this comment.
Why is this important? The difference between _d_arraysetlengthT and _d_arraysetlengthiT lies in their initialisers (0 or T.init) not in mutability.
There was a problem hiding this comment.
I need to update it via isMutable param where they are called right ?
| ._d_arraysetlengthiT(ti, newlength, cast(void[]*)&arr); | ||
| // Allocate new array for immutable/const | ||
| auto tempArr = new U[newlength]; | ||
|
|
||
| return arr.length; | ||
| // Copy existing elements | ||
| if (arr.ptr !is null) | ||
| { | ||
| memmove(tempArr.ptr, arr.ptr, arr.length * U.sizeof); | ||
| } | ||
|
|
||
| // Ensure initialization of new elements for immutable types | ||
| foreach (i; arr.length .. newlength) | ||
| { | ||
| emplace(&tempArr[i], U.init); | ||
| } | ||
|
|
||
| arr = cast(Tarr) tempArr; | ||
| return arr.length; | ||
| } | ||
|
|
There was a problem hiding this comment.
Don't allocate an extra array. The old hook tried to reallocate it using gc_expandArrayUsed. Keep the same inner logic.
| static size_t mulu(size_t a, size_t b, ref bool overflow) | ||
| { | ||
| size_t result = a * b; | ||
| overflow = (b != 0 && result / b != a); | ||
| return result; | ||
| } | ||
|
|
||
| newsize = mulu(sizeelem, newlength, overflow); | ||
| if (overflow) | ||
| { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
The whole point of the inline assembly code in the original hook was to verify the overflow using setc as opposed to that division. I'll guess the compiler can optimise your code to the same assembly, do these experiments once everything else passes.
shivangshukla7020
force-pushed
the
templating
branch
from
a4a790d to
001893d
Compare
4 participants
Description
This pull request refactors the
_d_arraysetlengthTand_d_arraysetlengthiTruntime hooks to use template-based implementations instead of relying onTypeInfo. This change improves performance by enabling compile-time specialization, reducing runtime overhead, and ensuring type safety.Key Changes
_d_arraysetlengthTand_d_arraysetlengthiTas templated functions.__arrayAlloc,memcpy,memmove, andemplace.constarrays.scope.dandtest23874.d._d_arraycatnTXto use the newarraysetlengthTfunction.This update enhances runtime efficiency while maintaining compatibility with existing functionality.
Kindly review @teodutu