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

Turn static void array into ubyte one #778

Merged
merged 1 commit into from Aug 12, 2014

Conversation

Projects
None yet
3 participants
@mihails-strasuns
Contributor

mihails-strasuns commented Aug 12, 2014

I am surprised this compiles with dmd but latest LDC alpha (also 2.065 frontend) reports:

Running ldmd2...
../../../../.dub/packages/vibe-d-0.7.21-alpha.3/source/vibe/core/core.d(1144): Error: static arrays of voids have no default initializer

Which makes sense. void does not have any size and having static array of those is totally undefined.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 12, 2014

Member

void[] is supposed to trigger the GC to search for pointers in this case. I didn't go through the code in detail, but it's possible that this change would introduce race conditions that result in dangling pointers. Why not change it to void[maxTaskParameterSize] args = void; instead, or does that still not compile with LDC?

Member

s-ludwig commented Aug 12, 2014

void[] is supposed to trigger the GC to search for pointers in this case. I didn't go through the code in detail, but it's possible that this change would introduce race conditions that result in dangling pointers. Why not change it to void[maxTaskParameterSize] args = void; instead, or does that still not compile with LDC?

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Aug 12, 2014

Contributor

void[] is not the same as void[const] - this a a very common mistake. In absence of precise GC it only pays attention to type of heap-allocated data (because TypeInfo gets stored during new). Static array is a value type and thus its type does not matter for GC at all - it will always be scanned if owner memory gets scanned. So this change should make no difference in that regard.

I am _very_surprised though to discover that void.sizeof compiles and is equal to 1 - this is totally out of the line with what void type usually means. With that in mind adding explicit = void initializer may make more sense. It looks so weird though :O

Contributor

mihails-strasuns commented Aug 12, 2014

void[] is not the same as void[const] - this a a very common mistake. In absence of precise GC it only pays attention to type of heap-allocated data (because TypeInfo gets stored during new). Static array is a value type and thus its type does not matter for GC at all - it will always be scanned if owner memory gets scanned. So this change should make no difference in that regard.

I am _very_surprised though to discover that void.sizeof compiles and is equal to 1 - this is totally out of the line with what void type usually means. With that in mind adding explicit = void initializer may make more sense. It looks so weird though :O

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 12, 2014

Member

My impression is that it should still mark the parent type as containing pointers, even for an imprecise GC, but if nothing else, it would still be worth to use as documentation for developers. Of course using void[] for this was a slightly strange/inconsistent choice in the first place, but if void.sizeof wouldn't be 1, slicing a void[] or using new void[n] would all break in absence of a lot of language special cases. It's still strange and I think a library defined type that is recognized by the GC would be a lot cleaner.

Member

s-ludwig commented Aug 12, 2014

My impression is that it should still mark the parent type as containing pointers, even for an imprecise GC, but if nothing else, it would still be worth to use as documentation for developers. Of course using void[] for this was a slightly strange/inconsistent choice in the first place, but if void.sizeof wouldn't be 1, slicing a void[] or using new void[n] would all break in absence of a lot of language special cases. It's still strange and I think a library defined type that is recognized by the GC would be a lot cleaner.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Aug 12, 2014

Contributor

Yeah but it will do so even if you use ubyte[const]. Basically it only doesn't get scanned when you get memory via new ubute[const].

Anyway, I have changed it to use = void.

Contributor

mihails-strasuns commented Aug 12, 2014

Yeah but it will do so even if you use ubyte[const]. Basically it only doesn't get scanned when you get memory via new ubute[const].

Anyway, I have changed it to use = void.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 12, 2014

Member

So you mean that when you do this:

struct S1 { ubyte[8] something; }
auto s = new S1;

The memory s points to gets scanned for pointers? That sounds quite inefficient and prone to avoidable false pointers.

/Thanks for confirming the = void change!

Member

s-ludwig commented Aug 12, 2014

So you mean that when you do this:

struct S1 { ubyte[8] something; }
auto s = new S1;

The memory s points to gets scanned for pointers? That sounds quite inefficient and prone to avoidable false pointers.

/Thanks for confirming the = void change!

s-ludwig added a commit that referenced this pull request Aug 12, 2014

Merge pull request #778 from Dicebot/void-static-arr
Turn static void array into ubyte one

@s-ludwig s-ludwig merged commit 67eaca7 into vibe-d:master Aug 12, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Aug 12, 2014

Contributor

The memory s points to gets scanned for pointers?

Yes, this is how it works according to my knowledge of GC internals. GC simply has not way to figure out that there is ubyte[8] memory block inside s. This is pretty much why precise GC is a cool thing.

Contributor

mihails-strasuns commented Aug 12, 2014

The memory s points to gets scanned for pointers?

Yes, this is how it works according to my knowledge of GC internals. GC simply has not way to figure out that there is ubyte[8] memory block inside s. This is pretty much why precise GC is a cool thing.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Aug 12, 2014

Contributor

btw linkedTravis PR job seems to be broken, can't even fetch git stuff

Contributor

mihails-strasuns commented Aug 12, 2014

btw linkedTravis PR job seems to be broken, can't even fetch git stuff

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 12, 2014

Member

I always thought that the new'd type would be inspected if it contains any pointers and if yes, the whole block of memory is marked as containing pointers. A precise GC would help for mixed blocks of memory, but a block not containing any pointers shouldn't be a problem to handle.

btw linkedTravis PR job seems to be broken, can't even fetch git stuff

Yeah, the outcome of the Travis-CI job is pretty random since a while. Sometimes it fails for basic setup stuff and often the compiler gets killed when building one of the examples - seems to be issues of the build machines on which the job runs rather than the job itself. Currently I simply don't have the time to look into it, but my fix would probably be to switch away from Travis and make my own CI results public.

Member

s-ludwig commented Aug 12, 2014

I always thought that the new'd type would be inspected if it contains any pointers and if yes, the whole block of memory is marked as containing pointers. A precise GC would help for mixed blocks of memory, but a block not containing any pointers shouldn't be a problem to handle.

btw linkedTravis PR job seems to be broken, can't even fetch git stuff

Yeah, the outcome of the Travis-CI job is pretty random since a while. Sometimes it fails for basic setup stuff and often the compiler gets killed when building one of the examples - seems to be issues of the build machines on which the job runs rather than the job itself. Currently I simply don't have the time to look into it, but my fix would probably be to switch away from Travis and make my own CI results public.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Aug 12, 2014

Contributor

Check https://github.com/D-Programming-Language/druntime/blob/master/src/gc/gc.d#L419 - problem is that GC only knows TypeInfo, it has no other tools for introspection of allocated type. Some help from compiler is needed but defining a system for passing such information to GC is a good chunk of implementation of precise GC.

My knowledge may be obsolete of course, I don't have courage to check compiler sources right now :)

Contributor

mihails-strasuns commented Aug 12, 2014

Check https://github.com/D-Programming-Language/druntime/blob/master/src/gc/gc.d#L419 - problem is that GC only knows TypeInfo, it has no other tools for introspection of allocated type. Some help from compiler is needed but defining a system for passing such information to GC is a good chunk of implementation of precise GC.

My knowledge may be obsolete of course, I don't have courage to check compiler sources right now :)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 12, 2014

Member

How about https://github.com/D-Programming-Language/druntime/blob/master/src/rt/lifetime.d#L1023 - looks like there is a flag stored in the TypeInfo for this. Of course in best undocumented magic number style ;)

Member

s-ludwig commented Aug 12, 2014

How about https://github.com/D-Programming-Language/druntime/blob/master/src/rt/lifetime.d#L1023 - looks like there is a flag stored in the TypeInfo for this. Of course in best undocumented magic number style ;)

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 12, 2014

Contributor

How about https://github.com/D-Programming-Language/druntime/blob/master/src/rt/lifetime.d#L1023 - looks like there is a flag stored in the TypeInfo for this.

Yes, I looked into this as well before. Every typeinfo has a NO_SCAN attribute for the GC when used with new. So if you have a an array of longs, the allocated long data block will be NO_SCAN and the GC will skip over it during mark'ing process, only the array pointer itself will be marked. That's why you should use void* arrays instead if the data should contain pointers.

Contributor

etcimon commented Aug 12, 2014

How about https://github.com/D-Programming-Language/druntime/blob/master/src/rt/lifetime.d#L1023 - looks like there is a flag stored in the TypeInfo for this.

Yes, I looked into this as well before. Every typeinfo has a NO_SCAN attribute for the GC when used with new. So if you have a an array of longs, the allocated long data block will be NO_SCAN and the GC will skip over it during mark'ing process, only the array pointer itself will be marked. That's why you should use void* arrays instead if the data should contain pointers.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 12, 2014

Contributor

Yes, this is how it works according to my knowledge of GC internals. GC simply has not way to figure out that there is ubyte[8] memory block inside s.

This wouldn't be scanned, it would see 8 consecutive ubyte elements but no pointer. My understanding is, precise GC is useful to avoid scanning the ubyte[8] if the struct/class wasn't NO_SCAN because of a pointer somewhere else in it.

e.g. With the following structure, you end up mark'ing all the data as if they were pointers because the class has a couple pointers in it: class MyData { MyData prev; MyData next; ubyte[1024] data; }

However, further indirections in the struct are admissible to another NO_SCAN flag because they're separate memory allocations. I can see here that a void[] forces the struct containing it to be scanned entirely: https://github.com/D-Programming-Language/druntime/blob/master/src/rt/typeinfo/ti_void.d
https://github.com/D-Programming-Language/druntime/blob/master/src/rt/typeinfo/ti_Ag.d#L99

Contributor

etcimon commented Aug 12, 2014

Yes, this is how it works according to my knowledge of GC internals. GC simply has not way to figure out that there is ubyte[8] memory block inside s.

This wouldn't be scanned, it would see 8 consecutive ubyte elements but no pointer. My understanding is, precise GC is useful to avoid scanning the ubyte[8] if the struct/class wasn't NO_SCAN because of a pointer somewhere else in it.

e.g. With the following structure, you end up mark'ing all the data as if they were pointers because the class has a couple pointers in it: class MyData { MyData prev; MyData next; ubyte[1024] data; }

However, further indirections in the struct are admissible to another NO_SCAN flag because they're separate memory allocations. I can see here that a void[] forces the struct containing it to be scanned entirely: https://github.com/D-Programming-Language/druntime/blob/master/src/rt/typeinfo/ti_void.d
https://github.com/D-Programming-Language/druntime/blob/master/src/rt/typeinfo/ti_Ag.d#L99

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Aug 12, 2014

Contributor

if the struct/class wasn't NO_SCAN because of a pointer somewhere else in it.

I have probably misunderstood original question from @s-ludwig - this is indeed correct. GC has no tools to treat any non-heap member in a special way - so it only will be marked as NO_SCAN if whole struct can be marked as NO_SCAN and even single pointer field (func function pointer in this PR) will result in scanning even byte static array fields.

Contributor

mihails-strasuns commented Aug 12, 2014

if the struct/class wasn't NO_SCAN because of a pointer somewhere else in it.

I have probably misunderstood original question from @s-ludwig - this is indeed correct. GC has no tools to treat any non-heap member in a special way - so it only will be marked as NO_SCAN if whole struct can be marked as NO_SCAN and even single pointer field (func function pointer in this PR) will result in scanning even byte static array fields.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Aug 13, 2014

Contributor

Ah, though I may be wrong here - it is plain function pointer after all, not a closure, so there is not point in scanning it.

Contributor

mihails-strasuns commented Aug 13, 2014

Ah, though I may be wrong here - it is plain function pointer after all, not a closure, so there is not point in scanning it.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 13, 2014

Contributor

Ah, though I may be wrong here - it is plain function pointer after all, not a closure, so there is not point in scanning it.

Yes, that would be here:

https://github.com/D-Programming-Language/druntime/blob/master/src/object_.d#L695

No flags for functions

Contributor

etcimon commented Aug 13, 2014

Ah, though I may be wrong here - it is plain function pointer after all, not a closure, so there is not point in scanning it.

Yes, that would be here:

https://github.com/D-Programming-Language/druntime/blob/master/src/object_.d#L695

No flags for functions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment