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

Memoize initializerSemantic #16886

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

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Sep 26, 2024

I found that on large global variables, the compiler spends lots of time creating GC pointer bitmaps by instantiating these templates in object.d:

template RTInfoImpl(size_t[] pointerBitmap)
{
    immutable size_t[pointerBitmap.length] RTInfoImpl = pointerBitmap[];
}

template NoPointersBitmapPayload(size_t N)
{
    enum size_t[N] NoPointersBitmapPayload = 0;
}

template RTInfo(T)
{
    enum pointerBitmap = __traits(getPointerBitmap, T);
    static if (pointerBitmap[1 .. $] == NoPointersBitmapPayload!(pointerBitmap.length - 1))
        enum RTInfo = rtinfoNoPointers;
    else
        enum RTInfo = RTInfoImpl!(pointerBitmap).ptr;
}

A reduced test case is:

enum uint[N] E(uint N) = 0;
immutable x = E!1000_000;

With templates, initializerSemantic is run twice, performing ctfe on a huge ArrayLiteralExp twice as well. semantic() should be an idempotent function, and 'semanticDone' flags are already in use on Dsymbol and Expression, so this PR extends that to Initializer.

@dlang-bot
Copy link
Contributor

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

@dkorpel dkorpel marked this pull request as ready for review September 26, 2024 15:34
@ibuclaw
Copy link
Member

ibuclaw commented Sep 26, 2024

Could the existing PASS flags type be used instead of a bool?

@dkorpel
Copy link
Contributor Author

dkorpel commented Sep 26, 2024

It could, but that would just add more possible invalid states to Initializer.

@@ -38,6 +38,7 @@ extern (C++) class Initializer : ASTNode
{
Loc loc;
InitKind kind;
bool semanticDone = false; /// initializerSemantic has been run on this
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is a performance change, I would like to see some numbers about the tradeoff in adding additional members to a common ASTNode.

Alternately steal an unused bit from kind for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some internal dub projects at SARC I tested, it shaves off about 0.8s of debug build time (19.1s => 18.3s). I doubt increasing the class instance size from 21 to 22 is a problem, it probably gets rounded up to 24 anyways because of alignment, but I can measure memory usage later.

Copy link
Contributor Author

@dkorpel dkorpel Sep 27, 2024

Choose a reason for hiding this comment

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

I just noticed that ArrayInitializer already has a now redundant sem field, and CInitializer has an unused sem field. Since I removed them now, memory usage might even be reduced, since those field where actually at the end, while the new semanticDone field sits in an alignment hole, since most derived classes start with a class field with alignment 8.

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.

4 participants