Refactor `TemplateInstance` to reduce its size #11368

andralex · 2020-07-03T23:40:13Z

This reduces the size of TemplateInstance from 384 bytes to 375 bytes. It does not affect compilation times or memory consumed in my tests on a large project. The impact will come if combined with other size improvements.

dlang-bot · 2020-07-03T23:40:14Z

Thanks for your pull request, @andralex!

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#11368"

src/dmd/dtemplate.d

thewilsonator

Need to update headers

andralex · 2020-07-03T23:53:15Z

@thewilsonator will update the C++ headers once review settles, thanks! @MoonlightSentinel minded your review, thanks.

ghost · 2020-07-04T02:00:46Z

from 384 bytes to 375 bytes. It does not affect compilation times or memory

This is because you must reduce the size down to 368 to see an impact (which makes 23 blocks exactly). inuse can be declared as ubyte, which makes 372. If in addition you manage to pack 4 more bytes this would be impactful.

andralex · 2020-07-04T02:04:59Z

OK, so following the recent comments I made inuse one byte large and kept nest 16 bits, of which 3 are taken by flags. Works?

andralex · 2020-07-04T02:08:05Z

Size is 371 now.

ghost · 2020-07-04T02:27:37Z

Sorry i was thinking modifications were in TemplateDeclaration hence my previous message could be confusing.
I think that 368 will not be possible then.

ghost · 2020-07-04T02:45:35Z

Maybe the solution: inherited member ScopeDsymbol.enflinnum is assigned plenty of times but never used. If you remove it then TemplateInstance size is 363. Also it wont be necessary to use inuse memory to store the set of Flag.

andralex · 2020-07-04T03:35:52Z

Maybe the solution: inherited member ScopeDsymbol.enflinnum is assigned plenty of times but never used. If you remove it then TemplateInstance size is 363. Also it wont be necessary to use inuse memory to store the set of Flag.

Interesting, one step at a time though.

andralex · 2020-07-04T03:52:30Z

@NilsLankila eh, removed endlinnum per your advice. Let's see what @WalterBright says.

ghost · 2020-07-04T04:15:26Z

I've tested in another PR (#11369). Maybe you're not browsing using the website but the test suite is broken.
BTW endlinnum was added in #2867

andralex · 2020-07-04T04:19:35Z

I've tested in another PR (#11369). Maybe you're not browsing using the website but the test suite is broken.

OK thanks, I undid my related commit.

src/dmd/dtemplate.d

ghost · 2020-07-04T10:15:42Z

@rainers has confirmed that ScopeDsymbol.endlinnum should not be removed, although it's most of the time unsued but when debug info are generated (and codeview only !). Sorry for the wrong track.

src/dmd/dtemplate.d

dnadlinger · 2020-07-04T19:43:58Z

The C++ headers need updating as well for the changed layout. (None of the fields seem to be used directly from LDC this time, so this is just a matter of avoiding ABI issues down the road.)

andralex · 2020-07-04T21:00:22Z

The C++ headers need updating as well for the changed layout. (None of the fields seem to be used directly from LDC this time, so this is just a matter of avoiding ABI issues down the road.)

Will do as soon as we settle on the layout.

andralex · 2020-07-05T19:52:05Z

Operated change on the C++ side as well (blindly), any mistake? Any other places to change?

dnadlinger · 2020-07-05T20:07:55Z

Operated change on the C++ side as well (blindly), any mistake? Any other places to change?

Don't think so. You could probably drop the C++ implementations as well; it's unlikely that these members are even used from C++ land.

src/dmd/dtemplate.d

ibuclaw

Apart from small nits also mentioned by @dnadlinger, I have some reservations about whether nest and the other bool flags really are mutually exclusive (what happens if I call toChars in the middle of a gdb session after one of the flags has been set?)

src/dmd/template.h

src/dmd/dtemplate.d

andralex · 2020-07-06T12:51:56Z

Apart from small nits also mentioned by @dnadlinger, I have some reservations about whether nest and the other bool flags really are mutually exclusive (what happens if I call toChars in the middle of a gdb session after one of the flags has been set?)

They can be used concomitantly. The taken bits are the uppermost ones, whereas the only way nest can be used is by incrementing and decrementing it, which doesn't affect the uppermost bits as long as it doesn't grow too much.

ibuclaw · 2020-07-06T20:18:11Z

Apart from small nits also mentioned by @dnadlinger, I have some reservations about whether nest and the other bool flags really are mutually exclusive (what happens if I call toChars in the middle of a gdb session after one of the flags has been set?)

They can be used concomitantly. The taken bits are the uppermost ones, whereas the only way nest can be used is by incrementing and decrementing it, which doesn't affect the uppermost bits as long as it doesn't grow too much.

Oh, now I see. I'm Captain Slow today.

Headers updated

Refactor `TemplateInstance` to reduce its size merged-on-behalf-of: Andrei Alexandrescu <andralex@users.noreply.github.com>

UplinkCoder · 2020-08-28T22:56:52Z

This code is severely aesthetically displeasing.

UplinkCoder · 2020-08-28T23:02:01Z

src/dmd/dtemplate.d

+
+    extern(D) final @safe @property pure nothrow @nogc
+    {
+        ushort nest() const { return _nest & Flag.available; }


@andralex style mistake

Which one?
It's in line with https://dlang.org/dstyle.html (see "exceptions") and common pattern used all over in DMD/Druntime/Phobos.

UplinkCoder · 2020-08-28T23:02:13Z

src/dmd/dtemplate.d

+        bool semantictiargsdone() const { return (_nest & Flag.semantictiargsdone) != 0; }
+        void semantictiargsdone(bool x)
+        {
+            if (x) _nest |= Flag.semantictiargsdone;


@andralex style mistake

andralex requested review from ibuclaw and RazvanN7 as code owners July 3, 2020 23:40

andralex force-pushed the flagpack branch from 5b89b7f to 5eec39d Compare July 3, 2020 23:41

andralex commented Jul 3, 2020

View reviewed changes

src/dmd/dtemplate.d Outdated Show resolved Hide resolved

andralex force-pushed the flagpack branch from 5eec39d to ae9f2c6 Compare July 3, 2020 23:46

dlang-bot added the Severity:Bug Fix label Jul 3, 2020

Pack Booleans together, reduce nest to 16 bit

32c9dce

andralex force-pushed the flagpack branch from ae9f2c6 to 32c9dce Compare July 3, 2020 23:47

MoonlightSentinel reviewed Jul 3, 2020

View reviewed changes

src/dmd/dtemplate.d Outdated Show resolved Hide resolved

src/dmd/dtemplate.d Outdated Show resolved Hide resolved

src/dmd/dtemplate.d Outdated Show resolved Hide resolved

thewilsonator previously requested changes Jul 3, 2020

View reviewed changes

Review

30d248d

andralex added 2 commits July 3, 2020 22:01

Better yet, use 3 MSBs inside nest for flags

795ac4c

Tighten up integral sizes

8fa08dd

andralex force-pushed the flagpack branch from 66b03d0 to 8fa08dd Compare July 4, 2020 04:18

Geod24 changed the title ~~Flagpack~~ Refactor TemplateInstance to reduce its size Jul 4, 2020

rainers reviewed Jul 4, 2020

View reviewed changes

src/dmd/dtemplate.d Outdated Show resolved Hide resolved

Fix flag values to fit the word

95fa493

MoonlightSentinel reviewed Jul 4, 2020

View reviewed changes

src/dmd/dtemplate.d Outdated Show resolved Hide resolved

andralex requested a review from WalterBright as a code owner July 4, 2020 18:06

Protect nest against accidental assignment

40c4726

andralex force-pushed the flagpack branch from c7f470e to 40c4726 Compare July 4, 2020 18:18

WalterBright approved these changes Jul 5, 2020

View reviewed changes

Propagate layout change to the C++ code as well

12d56d9

rainers reviewed Jul 6, 2020

View reviewed changes

src/dmd/dtemplate.d Outdated Show resolved Hide resolved

ibuclaw reviewed Jul 6, 2020

View reviewed changes

src/dmd/template.h Outdated Show resolved Hide resolved

src/dmd/template.h Outdated Show resolved Hide resolved

src/dmd/dtemplate.d Outdated Show resolved Hide resolved

Review

24eb114

andralex force-pushed the flagpack branch from 698876e to 24eb114 Compare July 6, 2020 12:59

Make C++ interface narrower

f06e343

andralex force-pushed the flagpack branch from 3ca2880 to f06e343 Compare July 7, 2020 01:03

ibuclaw approved these changes Jul 7, 2020

View reviewed changes

andralex added the Merge:auto-merge-squash label Jul 7, 2020

dlang-bot merged commit 3d11183 into dlang:master Jul 7, 2020

ghost pushed a commit to WalterBright/dmd that referenced this pull request Aug 12, 2020

Refactor TemplateInstance to reduce its size (dlang#11368)

1b0a4b5

Refactor `TemplateInstance` to reduce its size merged-on-behalf-of: Andrei Alexandrescu <andralex@users.noreply.github.com>

UplinkCoder pushed a commit to UplinkCoder/dmd that referenced this pull request Aug 12, 2020

Refactor TemplateInstance to reduce its size (dlang#11368)

1611414

Refactor `TemplateInstance` to reduce its size merged-on-behalf-of: Andrei Alexandrescu <andralex@users.noreply.github.com>

UplinkCoder reviewed Aug 28, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `TemplateInstance` to reduce its size #11368

Refactor `TemplateInstance` to reduce its size #11368

andralex commented Jul 3, 2020

dlang-bot commented Jul 3, 2020 •

edited

Loading

thewilsonator left a comment

andralex commented Jul 3, 2020

ghost commented Jul 4, 2020

andralex commented Jul 4, 2020 •

edited

Loading

andralex commented Jul 4, 2020

ghost commented Jul 4, 2020

ghost commented Jul 4, 2020 •

edited by ghost

Loading

andralex commented Jul 4, 2020

andralex commented Jul 4, 2020

ghost commented Jul 4, 2020 •

edited by ghost

Loading

andralex commented Jul 4, 2020

ghost commented Jul 4, 2020 •

edited by ghost

Loading

dnadlinger commented Jul 4, 2020

andralex commented Jul 4, 2020

andralex commented Jul 5, 2020

dnadlinger commented Jul 5, 2020

ibuclaw left a comment

andralex commented Jul 6, 2020

ibuclaw commented Jul 6, 2020

UplinkCoder commented Aug 28, 2020

UplinkCoder Aug 28, 2020

wilzbach Aug 28, 2020

UplinkCoder Aug 28, 2020

Refactor TemplateInstance to reduce its size #11368

Refactor TemplateInstance to reduce its size #11368

Conversation

andralex commented Jul 3, 2020

dlang-bot commented Jul 3, 2020 • edited Loading

Bugzilla references

Testing this PR locally

thewilsonator left a comment

Choose a reason for hiding this comment

andralex commented Jul 3, 2020

ghost commented Jul 4, 2020

andralex commented Jul 4, 2020 • edited Loading

andralex commented Jul 4, 2020

ghost commented Jul 4, 2020

ghost commented Jul 4, 2020 • edited by ghost Loading

andralex commented Jul 4, 2020

andralex commented Jul 4, 2020

ghost commented Jul 4, 2020 • edited by ghost Loading

andralex commented Jul 4, 2020

ghost commented Jul 4, 2020 • edited by ghost Loading

dnadlinger commented Jul 4, 2020

andralex commented Jul 4, 2020

andralex commented Jul 5, 2020

dnadlinger commented Jul 5, 2020

ibuclaw left a comment

Choose a reason for hiding this comment

andralex commented Jul 6, 2020

ibuclaw commented Jul 6, 2020

UplinkCoder commented Aug 28, 2020

UplinkCoder Aug 28, 2020

Choose a reason for hiding this comment

wilzbach Aug 28, 2020

Choose a reason for hiding this comment

UplinkCoder Aug 28, 2020

Choose a reason for hiding this comment

Refactor `TemplateInstance` to reduce its size #11368

Refactor `TemplateInstance` to reduce its size #11368

dlang-bot commented Jul 3, 2020 •

edited

Loading

andralex commented Jul 4, 2020 •

edited

Loading

ghost commented Jul 4, 2020 •

edited by ghost

Loading

ghost commented Jul 4, 2020 •

edited by ghost

Loading

ghost commented Jul 4, 2020 •

edited by ghost

Loading