[Issue 5813] [patch] std.array.Appender has severe performance and memory leak problems.

[d-bugmail at puremagic.com]

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



--- Comment #4 from Rob Jacques <sandford at jhu.edu> 2011-06-09 12:40:52 PDT ---
(In reply to comment #3)
> I think this is a good idea to do, but the code is somewhat obfuscated.  For
> example, you are returning a value from a void function (extend), and doing
> some weird shortcuts (like !false*2).  There are also some style differences
> from the current D style guidelines.  These will have to be fixed before it's
> put into phobos.

The obfuscation generally comes from a desire to minimize multi-lines to
one-liners. But if you think these are too compressed, I'll change them to
something cleaner. Regarding D style guidelines, there has been so much
discussion over them, that I don't know if/where there current guidelines
exist/are. And the existing Phobos codebase is a mash-up of different styles. I
mean, even ddoc comments vary between /++ and /**. Is there anything in
particular you'd recommend I'd change?

> One technical point -- the limit of multiples of page sizes when requesting
> more than one page is already done by the GC.  That is, If you request more
> than 1/2 page, you will get a multiple of pages.  I think the code will perform
> exactly as well if the multiple-of-page limitation is removed from your code,
> and this then does not make any assumptions about how big a page size is (if it
> ever changes).

Thank you. I'll remove that line.

> Do you want to do a pull request and have the code reviewed?  I think it's
> definitely worth changing Appender to do this.

Yes, but I haven't gotten around to learning/installing Git on Windows yet.

> One further thing -- I planned on making two versions of Appender: one that is
> a reference type, and one that is not.  The version that is not should allow
> filling an existing an array without allocating anything.  The current Appender
> (the one in phobos now) always allocates an pImpl struct, even if the array
> never needs to be extended.
> 
> The tradeoff is that the non-reference appender is not returnable, and you
> can't make copies of it (i.e. this(this) is @disabled).
> 
> Is this possible with your code?  If so, and it's easy enough, can you create a
> version that does this too?

The simple solution would be to emplace the first data node inside the appender
struct. But I'm not sure the benefit of such a solution: appender always
checks/extends-to the capacity of an array, which involves taking the GC lock,
etc. So since the taking lock can't be averted, avoiding an extra 16-byte
allocation isn't much worse. But I can always implement it using a template
parameter + static ifs, so it shouldn't be hard to try it out. Hmm... I could
also fold in RefAppender that way.

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