Skip to content

place statements array inside CompoundStatements, it is non-empty most of the time. - #23421

Merged
thewilsonator merged 5 commits into
dlang:masterfrom
rainers:statements_direct
Jul 29, 2026
Merged

place statements array inside CompoundStatements, it is non-empty most of the time.#23421
thewilsonator merged 5 commits into
dlang:masterfrom
rainers:statements_direct

Conversation

@rainers

@rainers rainers commented Jul 18, 2026

Copy link
Copy Markdown
Member

reduces size and avoids an extra allocation.

Also avoids the indirection on usage and makes the code a bit cleaner.

The downside is that constructing the CompoundStatement with an array of statements has to move the contents of the array if we don't want the extra copy - which might not be obvious from the code. This could be done a bit nicer with rvalues, but the bootstrap compilers don't have that.

@rainers
rainers requested a review from ibuclaw as a code owner July 18, 2026 19:15
@rainers
rainers marked this pull request as draft July 18, 2026 19:15
@github-actions

github-actions Bot commented Jul 18, 2026

Copy link
Copy Markdown

DMD perf check

Metric Base PR delta
compile hello.d (instr) 215.8 M 215.7 M -0.01%
compile hello.d -O (instr) 234.3 M 234.3 M 0.00%
compile Phobos (instr) 5,080.4 M 5,077.5 M -0.06%
dmd binary size (stripped) 6.88 MB 6.91 MB +0.34%
hello binary size 0.72 MB 0.72 MB 0.00%
peak RSS (compile hello.d) 44 MB 44 MB +0.02%
peak RSS (compile Phobos) 633 MB 631 MB -0.31%

@rainers
rainers force-pushed the statements_direct branch 3 times, most recently from 4943cef to 6e13454 Compare July 19, 2026 10:02
@rainers
rainers force-pushed the statements_direct branch from a7d86b6 to a4a12a1 Compare July 20, 2026 04:54
@rainers

rainers commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

The downside is that constructing the CompoundStatement with an array of statements has to move the contents of the array if we don't want the extra copy - which might not be obvious from the code. This could be done a bit nicer with rvalues, but the bootstrap compilers don't have that.

I could workaround dmd 2.079 not doing the return-value-optimizations by an additional ctor, so Statements are now explicitly moved into CompoundStatements and UnrolledLoopStatements.

@thewilsonator

Copy link
Copy Markdown
Contributor

Is this good to go?

- statementSemanticVisit.visitScope: don't call flatten for CompoundStatements, it just makes an unnecessary copy
@rainers

rainers commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

Is this good to go?

I wasn't too happy about statementSemanticVisit.visitScope moving the statements out of a CompoundStatement after a call to flatten instead of just sharing them with the original statement, just to notice that the new Statement is unnecessary in that case anyway, see https://github.com/dlang/dmd/pull/23421/changes#diff-11fb0212c55c05ed6e743e6776abb3c56996381760c8725255c622f96f5d9d4bR561
So a couple of more MBs saved.

If the changes are not considered too disturbing, I'm fine with it now.

@rainers
rainers marked this pull request as ready for review July 22, 2026 06:52
@rainers rainers changed the title Draft: place statements array inside CompoundStatements, it is non-empty most of the time. place statements array inside CompoundStatements, it is non-empty most of the time. Jul 22, 2026
@thewilsonator

Copy link
Copy Markdown
Contributor

If you can version out a fix that is not too intrusive for 2.079 that would be great.

@rainers

rainers commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

If you can version out a fix that is not too intrusive for 2.079 that would be great.

Ok, done. The single Array!T.move() function is not a lot shorter, though.

@rainers
rainers force-pushed the statements_direct branch from b8dfc29 to 6704c62 Compare July 23, 2026 06:42
@thewilsonator
thewilsonator merged commit 5b0ca3c into dlang:master Jul 29, 2026
42 checks passed
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.

2 participants