http://d.puremagic.com/issues/show_bug.cgi?id=3463


nfxjfg@gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #696 is|0                           |1
           obsolete|                            |


--- Comment #60 from nfxjfg@gmail.com 2010-07-28 08:40:37 PDT ---
Created an attachment (id=701)
D1 - patch for Tango's runtime to enable precise GC scanning

- lots of nasty refactoring in gcx.d:
   - kill the string mixin
   - replace binSize by binsize as suggested by Leandro (that was from
dsimcha's patch; no idea why it was there)
   - realloc is now "emulated" (this simplifies things a lot - untested because
neither the runtime, Tango, or any of my code actually use realloc); note that
realloc doesn't shrink anymore, but that could be added back
   - replace some the implementation of sizeOf and addrOf functions by using
Gcx.getInfo (the high level of code duplication in gcx.d sure is horrible)
   - explicitly support SENTINEL (I have no idea why the code apparently worked
with SENTINEL enabled; at least it should have messed up the bitmask scanning
offset)
   - now you can control per environment variable if bitmasks will be stored in
memory blocks (see Memory.d GC ddoc comment)
- update method names and documentation as suggested by Leandro in comment 44


Also, shouldn't functions like freeNoSync check for interior pointers? What happens if you call it with such a pointer?

There was also some awkwardness about sentinels. For example getInfo() doesn't substract the size of the sentinel from the returned size. (I fixed that specific one because not fixing it would have meant to introduce a special case.)

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=3463



--- Comment #61 from Leandro Lucarella <llucax@gmail.com> 2010-07-28 12:23:01 PDT ---
(In reply to comment #60)
> Created an attachment (id=701) [details]
> D1 - patch for Tango's runtime to enable precise GC scanning
> 
> - lots of nasty refactoring in gcx.d:

Even when I agree that the GC needs a lot of refactoring, I don't think it's a good idea to include it in this patch, it makes much harder to understand it and it might introduce some subtle bugs that will be very hard to track down in so many changes.

I'm working on a GC that was based on the Tango (0.99.9) GC and doing a lot of refactoring myself. The refactoring of things introduced by the patch looks really nice though.

> Also, shouldn't functions like freeNoSync check for interior pointers? What happens if you call it with such a pointer?

I don't think so, it should be undefined behavior (based on C's free()).

Thanks!

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=3463



--- Comment #62 from nfxjfg@gmail.com 2010-07-28 12:49:49 PDT ---
(In reply to comment #61)
> Even when I agree that the GC needs a lot of refactoring, I don't think it's a good idea to include it in this patch, it makes much harder to understand it and it might introduce some subtle bugs that will be very hard to track down in so many changes.

I have to agree, but I did these changes in order to make storing a bitmask configurable, and to properly support SENTINEL. The changes the original patch did weren't very small either.

I see 4 options:
1. keep this anyway
2. keep the old gcx.d around and apply the changes to a new incarnation of
gcx.d, and let the user choose the GC implementation at startup or compile time
3. only accept the compiler patch, and wait for Leandro's new GC
4. revert to the previous version of my patch (of course I wouldn't like this
at all)

Which is it?

I also wonder about D2. The array appending issue makes porting the Tango patch non-trivial.

> > Also, shouldn't functions like freeNoSync check for interior pointers? What happens if you call it with such a pointer?
> 
> I don't think so, it should be undefined behavior (based on C's free()).

Then the code should probably check whether the pointer is valid, and abort() or throw an exception if not. I don't think intentionally introducing random behavior when you could avoid it is a good idea. Also the documentation comments in Memory.d seem to disagree with your assessment.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=3463



--- Comment #63 from Leandro Lucarella <llucax@gmail.com> 2010-07-28 12:57:35 PDT ---
(In reply to comment #60)
>    - explicitly support SENTINEL (I have no idea why the code apparently worked
> with SENTINEL enabled; at least it should have messed up the bitmask scanning
> offset)

BTW, I think MEMSTOMP is broken too (but in a non-destructive way, things work anyways). Is very hard to keep those features functional because they are so hard to use. I've made those 2 features configurable via env variables in my GC. I'll try to publish my repo in case anyone want to steal some ideas :)

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=3463



--- Comment #64 from Leandro Lucarella <llucax@gmail.com> 2010-07-28 13:04:07 PDT ---
(In reply to comment #62)
> (In reply to comment #61)
> > Even when I agree that the GC needs a lot of refactoring, I don't think it's a good idea to include it in this patch, it makes much harder to understand it and it might introduce some subtle bugs that will be very hard to track down in so many changes.
> 
> I have to agree, but I did these changes in order to make storing a bitmask configurable, and to properly support SENTINEL. The changes the original patch did weren't very small either.
> 
> I see 4 options:
> 1. keep this anyway
> 2. keep the old gcx.d around and apply the changes to a new incarnation of
> gcx.d, and let the user choose the GC implementation at startup or compile time
> 3. only accept the compiler patch, and wait for Leandro's new GC
> 4. revert to the previous version of my patch (of course I wouldn't like this
> at all)
> 
> Which is it?

I think this should be replied by whoever have the authority to merge the patches, my comments were just wishes (and you made them true in a much higher proportion that I was expecting :). I'd say 1 is OK.

About my GC, is a research work and, even when my goal is something realistic and I'm doing performance tests all the time, don't expect much. :)

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=3463



--- Comment #65 from Leandro Lucarella <llucax@gmail.com> 2010-08-07 22:19:43 PDT ---
This blog post might be of interesting for this bug report: http://www.llucax.com.ar/blog/blog/post/1490c03e

Specially for comment 55 and comment 56.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=3463



--- Comment #66 from bearophile_hugs@eml.cc 2010-08-08 06:03:55 PDT ---
> http://www.llucax.com.ar/blog/blog/post/1490c03e

That page shows me a spectacularly complex page of Python error messages.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=3463



--- Comment #67 from Leandro Lucarella <llucax@gmail.com> 2010-08-08 08:10:19 PDT ---
Sor(In reply to comment #66)
> > http://www.llucax.com.ar/blog/blog/post/1490c03e
> 
> That page shows me a spectacularly complex page of Python error messages.

Sorry about that, fixed!

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=3463



--- Comment #68 from Leandro Lucarella <llucax@gmail.com> 2010-08-15 07:24:36 PDT ---
More analysis on wasted space (for the current GC and for the precise patch)
here:
http://www.llucax.com.ar/blog/blog/post/098ceae8

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=3463


nfxjfg@gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #701 is|0                           |1
           obsolete|                            |


--- Comment #69 from nfxjfg@gmail.com 2010-08-24 01:37:05 PDT ---
Created an attachment (id=737)
 D1 - patch for Tango's runtime to enable precise GC scanning

Same as old patch, updated to newer tango svn.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------