[D-runtime] [phobos] Fix for array append cache holding data hostage

[Steve Schveighoffer]

Well, looks like there is no response to this, I'll commit.  I'll warn you 
however, that I have not tested on OSX.  Linux and Windows both pass unit tests.

-Steve



----- Original Message ----
> From: Steve Schveighoffer <schveiguy at yahoo.com>
> To: Phobos <phobos at puremagic.com>; d-runtime <d-runtime at puremagic.com>
> Sent: Thu, December 23, 2010 4:08:03 PM
> Subject: [phobos] Fix for array append cache holding data hostage
> 
> Cross-posting to both phobos and druntime for a wider audience...
> 
> For  those of you not familiar with how the array appending cache works, it 
> essentially caches recent lookups of BlkInfo for non-shared array  appends.
> 
> What this means is that appending to an unshared array does not  have to take 
>the 
>
> global GC lock, and allows a lookup to happen almost  immediately.
> 
> One of the things that the cache required was to keep that  array in memory.  
> Otherwise, there was a possibility that the array  would be collected and 
> reallocated.  An append to such an array would  result in the stale cache entry 
>
>
> being used as the block info, possibly  resulting in the wrong start address, 
> wrong size, or wrong flags, all of  which could be fatal.  A good prior 
> discussion for this issue is listed  in this bug: 
> http://d.puremagic.com/issues/show_bug.cgi?id=3929
> 
> In  order to solve the problem, I implemented a GC collection hook which runs 
> *after* the mark phase, but before all threads are resumed.  This hook  tests 
>all 
>
> cache entries to see if any of them are about to be  collected.  Any entries 
> which are not marked (i.e. about to be  collected) are removed from the cache.
> 
> I also had to do some funky stuff  with the cache since TLS data is also 
> scanned.  So the cache is now in  a malloc'd block that is freed on thread exit 
>
>
> (and lazily allocated on first  append).
> 
> One of the best tests that showed this problem (or so I thought)  was posted to 
>
>
> the newsgroup here: 
>http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D.learn&article_id=22715
>
>5
> 
> 
> However,  my patch does *not* fix this problem.  I've found that the true 
>reason 
>
> for the memory being held is that other data is pointing at the block.   With a 
>
>
> 200MB block, this is understandable.  In a 4GB address space,  that's 1/20 of 
>the 
>
> valid address space. Any random aligned 4-byte sequence  in the stack or TLS 
> could point to it and prevent it from being  collected.
> 
> The really bad thing about this is, once the true pointers to  that block are 
> gone, it could be lost forever.  The stack data which  "points" to it may be 
>very 
>
> high up on the stack, which means it never gets  collected.  It makes me wonder 
>
>
> if we might want to provide an unsafe  'realloc' type function for appending 
>that 
>
> forcefully frees blocks that were  used in an append loop.  A 'unique' type 
> qualifier could go a long way  here.
> 
> In any case, I still want to checkin this fix, because it at least  removes the 
>
>
> cache as a possible culprit (and fixes 3929).  I don't want  to commit the 
>patch 
>
> until people here agree on the timing.  There is a  possibility that it 
> introduces memory corruption bugs (this happened the  last time I was tinkering 
>
>
> with array appending), so I don't want to  destabilize everything if there is 
> good reason.  Given that we just had  a release, I think this would be a good 
> time to do it.  I most likely  won't get to it this weekend.
> 
> So anyone have any objections to me  checking this in?
> 
> -Steve
> 
> 
> 
>      
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
>