[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 #5 from Steven Schveighoffer <schveiguy at yahoo.com> 2011-06-09 14:22:32 PDT ---
(In reply to comment #4)
> The obfuscation generally comes from a desire to minimize multi-lines to
> one-liners.

I don't mind the one-liners, but things like !hasIndirections!T*2 is better
written as hasIndirections!T ? 0 : GC.BlkAttr.NO_SCAN.  It's more decipherable
and self-documenting.  Shortcuts that hide the intention of the code are not
worth it.  The one-liners that assign to several things at once are at least
clear.  Although I think this one should be split into at least two statements:

return _tail = _tail.next = new Data( size );

> 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?

I think the best thing to do for now is to follow the style of the original
code.  TBH, I'd say submit the pull request and the review will flesh out
individual things that should be fixed (the github review process is really
good).

> > 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.

Highly recommend progit.org for learning git.  Just getting git working on
Windows myself...

> 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.

You are probably right, I added the code to fill to capacity after thinking
about creating such a type.  The capacity check definitely needs to take the
global lock.  Maybe the non-reference appender would not try to expand to
capacity (that is, after all, an optimization).  In any case, this isn't as
important as the other fixes you've already created.

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