This is an archive of the discontinued LLVM Phabricator instance.

[Support] Make the default chunk size of raw_fd_ostream to 1 GiB.
ClosedPublic

Authored by ruiu on Oct 30 2017, 7:18 PM.

Details

Summary

Previously, we call write(2) for each 32767 byte chunk. That is not
efficient because Linux can handle much larger write requests.
This patch changes the chunk size on Linux to 1 GiB.

This patch doesn't change the existing behavior for Windows.

Event Timeline

ruiu created this revision.Oct 30 2017, 7:18 PM
zturner added inline comments.
llvm/lib/Support/raw_ostream.cpp
594–596

The comment here doesn't make sense to me. It says "Writing a large size of output to Windows console", but then the code only limits the write size if it's not a TTY. Isn't this check inverted? It should be setting the max write size if it is a tty.

Furthermore, the documentation for WriteConsole doesn't say anything about Windows 8, it just says the buffer is at most 64k. It even says that the buffer could be smaller, but how much smaller depends on heap usage. So for all we know, it might not even be 32K.

Granted, you're only changing the non-Windows code path, but this just looks wrong to me, in case you're interested in fixing it.

compnerd accepted this revision.Oct 30 2017, 7:32 PM
This revision is now accepted and ready to land.Oct 30 2017, 7:32 PM
compnerd added inline comments.Oct 30 2017, 7:35 PM
llvm/lib/Support/raw_ostream.cpp
594–596

Its a conversion trick (int to boolean) via double negation.

ruiu added inline comments.Oct 30 2017, 7:36 PM
llvm/lib/Support/raw_ostream.cpp
594–596

Actually it is !!, so it checks if isatty && Windows7OrOlder.

But I agree that this is confusing. I'll just remove !! as it doesn't seem necessary.

zturner added inline comments.Oct 30 2017, 7:37 PM
llvm/lib/Support/raw_ostream.cpp
594–596

Oh, duhh.. Now I feel stupid. I use the !! myself too, it just wasn't registering with me for some reason. The whole Windows 8 thing still seems dubious to me, since there's no reference (e.g. msdn link) about why this is specific to Windows 7 and below.

krytarowski added inline comments.Oct 30 2017, 7:41 PM
llvm/lib/Support/raw_ostream.cpp
587–590

Am I looking correctly that we enforce Linux behavior to every UNIX?

ruiu added inline comments.Oct 30 2017, 7:41 PM
llvm/lib/Support/raw_ostream.cpp
594–596

I think we can just remove !RunningWindows8OrGreater(), as limiting write size to 32 KiB is not that inefficient, and when we are writing to a console, console's speed is a bottleneck anyway. But that should be done in a separate patch.

ruiu added inline comments.Oct 30 2017, 7:43 PM
llvm/lib/Support/raw_ostream.cpp
587–590

Yes. Compared to writing 1 GiB data, the overhead of system call is negligible, so I kept it simple.

krytarowski added inline comments.Oct 30 2017, 7:46 PM
llvm/lib/Support/raw_ostream.cpp
587–590

Keeping it simple is fine, but I prompt to make this comment more aware of existence of other OSes... or even better just enable it on Linux, it might silently break others like Linux for >2GB.

ruiu updated this revision to Diff 120929.Oct 30 2017, 7:50 PM
  • Do it only for Linux.
ruiu added inline comments.Oct 30 2017, 7:50 PM
llvm/lib/Support/raw_ostream.cpp
587–590

Sure.

krytarowski added inline comments.Oct 30 2017, 7:54 PM
llvm/lib/Support/raw_ostream.cpp
580
If the value of nbyte is greater than {SSIZE_MAX}, the result is implementation-defined.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html#tag_16_685

ruiu added inline comments.Oct 30 2017, 7:59 PM
llvm/lib/Support/raw_ostream.cpp
580

Ah. Then, maybe that's what Saleem observed? The existing code could make a write request that is larger than SSIZE_MAX (= 2 GiB - 1) on a 32-bit machine.

If so, we can just remove the ifdef for Linux and use SSIZE_T as the default MaxWriteSize.

krytarowski added inline comments.Oct 30 2017, 8:07 PM
llvm/lib/Support/raw_ostream.cpp
580

SSIZE_MAX is POSIX only (unless it was added to C++, but I don't see a reference).

NetBSD specific behavior allows length up to SSIZE_MAX:

	/*
	 * Writes return ssize_t because -1 is returned on error.  Therefore
	 * we must restrict the length to SSIZE_MAX to avoid garbage return
	 * values.
	 */
	if (auio.uio_resid > SSIZE_MAX) {
		error = EINVAL;
		goto out;
}
  • src/sys/kern/sys_generic.c
krytarowski added inline comments.Oct 31 2017, 6:27 AM
llvm/lib/Support/raw_ostream.cpp
659

The same problem here. NetBSD disallows length larger than SSIZE_MAX and it will return EINVAL.

ruiu updated this revision to Diff 120999.Oct 31 2017, 9:40 AM
  • Limit the maximum write size to SSIZE_MAX.
ruiu added inline comments.Oct 31 2017, 9:41 AM
llvm/lib/Support/raw_ostream.cpp
580

I'm not really sure if we want to deal with limitations of SSIZE_MAX instead of just calling write for each 1 GiB of data, but I addressed your concern in the new patch.

659

This is not a call of ::write but write member function defined in the same class.

krytarowski accepted this revision.Oct 31 2017, 10:10 AM
This revision was automatically updated to reflect the committed changes.