Skip to content

Conversation

andralex
Copy link
Member

These changes make byLine more than 3x faster on OSX without changing semantics. Given that part of the changes involve paths taken by the other OSs, it's possible there's speedup elsewhere as well. Please measure, thanks!

ptrdiff_t getdelim(char**, size_t*, int, FILE*);
// getline() always comes together with getdelim()
ptrdiff_t getline(char**, size_t*, FILE*);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that be better to use a port of the actual C header of the plateform ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should update core.stdc.stdio and core.sys.posix.stdio.

@WalterBright
Copy link
Member

What's the bugzilla issue about this?

@rainers
Copy link
Member

rainers commented Mar 22, 2015

Please note that readlnImpl is currently broken: #2794

@ntrel
Copy link
Contributor

ntrel commented Mar 22, 2015

@MartinNowak
Copy link
Member

Apparently getdelim is a Posix function and should be available on many platforms.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/getline.html

if (n > 128 * 1024)
{
// Bound memory used by readln
free(lineptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with leaking this until program exit, but valgrind would report this as leak.
Adding a ~static this() {} might run into circular dependency issues though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fortunately we have std.stdiobase for that, where we can include a call to the module destructor as well.

@MartinNowak
Copy link
Member

Auto-merge toggled on

MartinNowak added a commit that referenced this pull request Mar 23, 2015
3x faster byLine implementation on OSX, probably faster on Linux as well
@MartinNowak MartinNowak merged commit 73b2426 into dlang:master Mar 23, 2015
@andralex
Copy link
Member Author

@MartinNowak: yah, though getdelim seems to be missing on a random bunch of platforms: https://www.gnu.org/software/gnulib/manual/html_node/getdelim.html. Are all of these old and irrelevant? If so then we can do away with all this HAS_GETDELIM business.

@andralex andralex deleted the byLine branch March 23, 2015 15:52
@MartinNowak
Copy link
Member

No idea, MSVC 9 does not seem to be to old, and some of the more obscure platforms like HP-UX lack this function but aren't supported anyhow. OSX10.5 and FBSD6 are really old by now.

@MartinNowak
Copy link
Member

Better keep it until we have updated the declarations in druntime.
BTW, you have an idea how to fix readln #2794, because it's the last open regression.

tramker pushed a commit to tramker/phobos that referenced this pull request Apr 7, 2015
3x faster byLine implementation on OSX, probably faster on Linux as well

Signed-off-by: Martin Krejcirik <mk@krej.cz>
tramker pushed a commit to tramker/phobos that referenced this pull request Apr 20, 2015
3x faster byLine implementation on OSX, probably faster on Linux as well

Signed-off-by: Martin Krejcirik <mk@krej.cz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants