Skip to content

Conversation

@rainers
Copy link
Member

@rainers rainers commented Dec 5, 2014

This helps the precise GC (dlang/druntime#1022) to generate the required data without adding to the compile time.

src/traits.c Outdated
Copy link

Choose a reason for hiding this comment

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

Holy mother of god. :D

Copy link
Member Author

Choose a reason for hiding this comment

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

As usual with ugly code, it was wrong ;-)

@ghost
Copy link

ghost commented Dec 6, 2014

Could you add a description either in the commit or the PR what exactly this does? A dlang.org pull will be needed documenting this trait as well (maybe just do that instead of documenting it in the commit).

@rainers rainers force-pushed the trait_getPointerBitmap branch from deaff10 to abcbf09 Compare December 6, 2014 17:58
@rainers
Copy link
Member Author

rainers commented Dec 6, 2014

Could you add a description either in the commit or the PR what exactly this does? A dlang.org pull will be needed documenting this trait as well (maybe just do that instead of documenting it in the commit).

I've added a comment to the code. I'll follow up with a documentation PR if this appears to have a chance of getting accepted.

@ghost
Copy link

ghost commented Dec 6, 2014

I have to admit that it's a bit obscure. But if you believe it will help in implementing a precise GC, I'm all for it. There's no way to implement this in the library instead, right?

@rainers
Copy link
Member Author

rainers commented Dec 6, 2014

This trait is a direct translation of the RTInfo in the precise GC: https://github.com/rainers/druntime/blob/gc_precise_nov14/src/gc/rtinfo.d

The problem with the library implementation is that it adds considerably to the compilation time (more than 20% for some projects). Having the proper RTInfo always available should have minimal impact on build time, especially in case you don't use the precise GC.

@ghost
Copy link

ghost commented Dec 6, 2014

Ok, you got me convinced. Will auto-merge soon, and hoping for that dlang pull as well. ;)

@rainers
Copy link
Member Author

rainers commented Dec 6, 2014

and hoping for that dlang pull as well. ;)

dlang/dlang.org#718

@ghost
Copy link

ghost commented Dec 6, 2014

Auto-merge toggled on

ghost pushed a commit that referenced this pull request Dec 6, 2014
Enhancement: add trait getPointerBitmap to help precise scanning
@ghost ghost merged commit 3a46cef into dlang:master Dec 6, 2014
@rainers
Copy link
Member Author

rainers commented Dec 6, 2014

Thanks!

@MartinNowak
Copy link
Member

Make sense to have this as a compiler intrinsic.

Copy link
Member

Choose a reason for hiding this comment

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

Just move that into main please, no other dmd test uses unittest.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have copied that from another test, but I shouldn't have used traits_getUnittests.d for that :-)
The bad thing about main() and building without -unittest is that asserts are a noop for all the -release builds.

@MartinNowak
Copy link
Member

Have you tested this with nested classes and structs?

@rainers
Copy link
Member Author

rainers commented Dec 6, 2014

Have you tested this with nested classes and structs?

TBH not with this trait. I'll add some test cases.

@rainers
Copy link
Member Author

rainers commented Dec 6, 2014

I'll add some test cases.

See #4196

@llucax
Copy link
Contributor

llucax commented Dec 7, 2014

Wow, we (mihails :P) just removed code using using a very similar approach in an old patch from @dsimcha in the draft of the port of the concurrent GC. Maybe it can be added right back :)

@9rnsr
Copy link
Contributor

9rnsr commented Dec 7, 2014

Why this PR is merged without @WalterBright 's approval?

@ghost
Copy link

ghost commented Dec 7, 2014

Because it's an enhancement that doesn't change the language, it only introduces a trait (there are no stolen keywords). And it seems important enough to have as a trait, especially since @rainers is one of the few people who actually know a thing or two about working on GCs.

Also, I'm not in the mood to wait for Walter to respond 5 months later for what is an uncontroversial feature.

@9rnsr
Copy link
Contributor

9rnsr commented Dec 7, 2014

@andralex OK, I understand.

@ghost
Copy link

ghost commented Dec 7, 2014

I mean I may have shotgun-pulled this, but GC is a high point of focus right now, we need to get this stuff moving. :)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants