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

Fix static AA initialization #15744

Merged
merged 4 commits into from Nov 2, 2023
Merged

Conversation

schveiguy
Copy link
Member

Static AA initialization (the yet unreleased feature, added in #15468) is broken for some cases of AA initialization.

The issue comes from TypeInfo.getHash not always behaving the same as hashOf. The fix here is to add a delegate which does the hashing to the Impl struct of the AA. Normal AA initialization at runtime copies the key's TypeInfo.getHash function, whereas when we initialize it using static AA initialization, we write our own function that always uses hashOf, making the hashing consistent.

FWIW, this means some cases where the compiler passes in the key TypeInfo are not needed, though we still call opEquals from the key TypeInfo. Possibly we can remove that usage by storing all the delegates needed upon creation.

@dlang-bot
Copy link
Contributor

dlang-bot commented Oct 29, 2023

Thanks for your pull request, @schveiguy!

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 "stable + dmd#15744"

@schveiguy
Copy link
Member Author

ping @dkorpel This is going to fail unittests, and I don't know why. I couldn't get it to pass on my local system with a double[2] key.

runtime. We do this by storing the hash function in the Impl instead of
using the passed-in TypeInfo for the key.
@maxhaton
Copy link
Member

Is there a bugzilla?

@schveiguy
Copy link
Member Author

As this is a new unreleased feature, I am unsure if it's a bug or not. I'm hoping @dkorpel can shed some light.

@schveiguy
Copy link
Member Author

Hm... an issue with the current DMD tests, I'll investigate.

{
assert(__ctfe, "makeAA Must only be called at compile time");
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this because I had to make this @trusted. Note that we can pretty much ignore all attributes because this is CTFE only.

@@ -642,7 +642,8 @@ class TypeInfo
*/
size_t getHash(scope const void* p) @trusted nothrow const
{
return hashOf(p);
// by default, do not assume anything about the type
return 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

Anyone have any idea why this would be bad? The default of hashing the pointer seems ludicrous to me -- the pointer comes from anywhere and is a consequence of having a runtime interface vs. a compile time one. It has nothing to do with item equality or hashing.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be a bad performance regression, not sure if anyone actually relies on it though.

@schveiguy
Copy link
Member Author

OK, finally this is the thing that fails on my local system.

If I move the static AA declaration outside the unittest, I get the error:

src/core/internal/newaa.d(158): Error: non-constant expression `[1.0, 2.0][0..2]`

@schveiguy
Copy link
Member Author

OK, I found a test that actually compiles and tests the change. unfortunately, it only works if I declare a global (this will be a different bug filed) instead of a static variable. So that's why I put in the version(unittest).

If this turns green, I think it's good to go.

@schveiguy
Copy link
Member Author

schveiguy commented Oct 30, 2023

What is this error with profilegc? What is the problem, and how do I fix it?

EDIT: I figured it out, and added a readme to help future me or others understand the issues.

Comment on lines +157 to +163
version(unittest):
struct Foo {
ubyte x;
double d;
}
int[Foo] utaa = [Foo(1, 2.0) : 5];
unittest {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version(unittest):
struct Foo {
ubyte x;
double d;
}
int[Foo] utaa = [Foo(1, 2.0) : 5];
unittest {
unittest
{
static struct Foo
{
ubyte x;
double d;
}
static int[Foo] utaa = [Foo(1, 2.0) : 5];

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll look into it

Copy link
Contributor

Choose a reason for hiding this comment

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

@schveiguy
Copy link
Member Author

@ibuclaw if you can hold off until this is merged to do the big merge, that would be good. I expect this to turn green.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 1, 2023

@ibuclaw if you can hold off until this is merged to do the big merge, that would be good. I expect this to turn green.

You can always just set the base to stable. :-)

@schveiguy
Copy link
Member Author

Just ping me if you do it before this is merged.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 1, 2023

Just ping me if you do it before this is merged.

I'm just starting off a final point release of stable, should occupy a couple hours before freezing master for 2.106 beta

@schveiguy
Copy link
Member Author

FWIW, I can't target stable with this until master is merged into stable, because I need Dennis' changes to the newaa as a base.

@schveiguy schveiguy changed the title Fix static AA initialization Draft: Fix static AA initialization Nov 1, 2023
@schveiguy schveiguy changed the title Draft: Fix static AA initialization Fix static AA initialization Nov 1, 2023
@schveiguy schveiguy marked this pull request as draft November 1, 2023 21:43
@schveiguy
Copy link
Member Author

Waiting for merge to stable, then I will retarget stable.

@ibuclaw ibuclaw changed the base branch from master to stable November 1, 2023 22:43
@ibuclaw
Copy link
Member

ibuclaw commented Nov 1, 2023

@schveiguy I've changed base of this PR to stable.

@schveiguy schveiguy marked this pull request as ready for review November 2, 2023 00:04
@schveiguy
Copy link
Member Author

OK, if nobody reviews this, I guess just merge it in 48 hours?

@thewilsonator thewilsonator merged commit 084eed6 into dlang:stable Nov 2, 2023
43 checks passed
@schveiguy schveiguy deleted the fixstaticaa branch November 2, 2023 12:31
@ibuclaw ibuclaw mentioned this pull request Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants