New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable FILE_FLAG_RANDOM_ACCESS in WindowsFileSystem #8522

Merged
merged 1 commit into from Mar 22, 2017

Conversation

Projects
None yet
7 participants
@snnn
Contributor

snnn commented Mar 18, 2017

You will run out of physical memory if you need to read a large file on Windows.
https://support.microsoft.com/en-us/help/2549369
The behavior looks very like a memory leak, however, the process's memory usage doesn't grow.

@tensorflow-jenkins

This comment has been minimized.

Show comment
Hide comment
@tensorflow-jenkins

tensorflow-jenkins Mar 18, 2017

Collaborator

Can one of the admins verify this patch?

Collaborator

tensorflow-jenkins commented Mar 18, 2017

Can one of the admins verify this patch?

@caisq

This comment has been minimized.

Show comment
Hide comment
@caisq

caisq Mar 18, 2017

Contributor

@tensorflow-jenkins test this please

Contributor

caisq commented Mar 18, 2017

@tensorflow-jenkins test this please

@snnn

This comment has been minimized.

Show comment
Hide comment
@snnn

snnn Mar 19, 2017

Contributor

Some performance test result after this change:

input data: A text file in libsvm format, 10906667862 bytes, 164540416 lines, with CRLF line terminators

test code:

#include <mutex>
#include <Windows.h>
#include <stdio.h>

#include "tensorflow/core/platform/env.h"
#include "tensorflow/core/lib/core/errors.h"
#include "tensorflow/core/lib/io/inputbuffer.h"
using namespace tensorflow;

int test1(const char* filename) {
	LARGE_INTEGER li;
	QueryPerformanceFrequency(&li);
	Env* env = Env::Default();
	std::unique_ptr<RandomAccessFile> file_;
	const ::tensorflow::Status status = env->NewRandomAccessFile(filename, &file_);
	if (!status.ok()) {
		return -1;
	}
	LARGE_INTEGER startTime, endTime;
	int kBufferSize = 2 * 1024 * 1024;
	io::InputBuffer* input_buffer = new io::InputBuffer(file_.get(), kBufferSize);
	QueryPerformanceCounter(&startTime);
	string line_contents;
	while (true) {
		Status status = input_buffer->ReadLine(&line_contents);
		if (errors::IsOutOfRange(status)) {
			break;
		}
		if (!status.ok()) {
			return -1;
		}
	}
	QueryPerformanceCounter(&endTime);
	const double seconds = ((endTime.QuadPart - startTime.QuadPart) / (double)li.QuadPart);
	printf("dur = %g\n", seconds);
	return 0;
}

int test2(const char* filename) {
	LARGE_INTEGER li;
	QueryPerformanceFrequency(&li);
	int kBufferSize = 2 * 1024 * 1024;
	std::unique_ptr<char[]> buf(new char[kBufferSize]);
	FILE* fd = fopen(filename, "rb");
	if (fd == nullptr) return -1;
	LARGE_INTEGER startTime, endTime;
	QueryPerformanceCounter(&startTime);
	while (fgets(buf.get(), kBufferSize, fd) != nullptr) {

	}
	QueryPerformanceCounter(&endTime);
	const double seconds = ((endTime.QuadPart - startTime.QuadPart) / (double)li.QuadPart);
	printf("read by fgets, dur = %g\n", seconds);
	return 0;
}

int main(int argc,char* argv[]) {
	const char* filename = argv[1];
	const char* method = argv[2];
	if (_stricmp(method, "fgets")==0) {
		return test2(filename);
	}
	return test1(filename);
}

Result:
file on SSD, read by fgets, dur = 35 seconds
file on SSD, read by InputBuffer, dur = 19 seconds
file on HardDisk, read by fgets, dur = 104 seconds
file on HardDisk, read by InputBuffer, dur = 114 seconds

So, I think it's ok to simply remove FILE_FLAG_RANDOM_ACCESS flag.

Contributor

snnn commented Mar 19, 2017

Some performance test result after this change:

input data: A text file in libsvm format, 10906667862 bytes, 164540416 lines, with CRLF line terminators

test code:

#include <mutex>
#include <Windows.h>
#include <stdio.h>

#include "tensorflow/core/platform/env.h"
#include "tensorflow/core/lib/core/errors.h"
#include "tensorflow/core/lib/io/inputbuffer.h"
using namespace tensorflow;

int test1(const char* filename) {
	LARGE_INTEGER li;
	QueryPerformanceFrequency(&li);
	Env* env = Env::Default();
	std::unique_ptr<RandomAccessFile> file_;
	const ::tensorflow::Status status = env->NewRandomAccessFile(filename, &file_);
	if (!status.ok()) {
		return -1;
	}
	LARGE_INTEGER startTime, endTime;
	int kBufferSize = 2 * 1024 * 1024;
	io::InputBuffer* input_buffer = new io::InputBuffer(file_.get(), kBufferSize);
	QueryPerformanceCounter(&startTime);
	string line_contents;
	while (true) {
		Status status = input_buffer->ReadLine(&line_contents);
		if (errors::IsOutOfRange(status)) {
			break;
		}
		if (!status.ok()) {
			return -1;
		}
	}
	QueryPerformanceCounter(&endTime);
	const double seconds = ((endTime.QuadPart - startTime.QuadPart) / (double)li.QuadPart);
	printf("dur = %g\n", seconds);
	return 0;
}

int test2(const char* filename) {
	LARGE_INTEGER li;
	QueryPerformanceFrequency(&li);
	int kBufferSize = 2 * 1024 * 1024;
	std::unique_ptr<char[]> buf(new char[kBufferSize]);
	FILE* fd = fopen(filename, "rb");
	if (fd == nullptr) return -1;
	LARGE_INTEGER startTime, endTime;
	QueryPerformanceCounter(&startTime);
	while (fgets(buf.get(), kBufferSize, fd) != nullptr) {

	}
	QueryPerformanceCounter(&endTime);
	const double seconds = ((endTime.QuadPart - startTime.QuadPart) / (double)li.QuadPart);
	printf("read by fgets, dur = %g\n", seconds);
	return 0;
}

int main(int argc,char* argv[]) {
	const char* filename = argv[1];
	const char* method = argv[2];
	if (_stricmp(method, "fgets")==0) {
		return test2(filename);
	}
	return test1(filename);
}

Result:
file on SSD, read by fgets, dur = 35 seconds
file on SSD, read by InputBuffer, dur = 19 seconds
file on HardDisk, read by fgets, dur = 104 seconds
file on HardDisk, read by InputBuffer, dur = 114 seconds

So, I think it's ok to simply remove FILE_FLAG_RANDOM_ACCESS flag.

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Mar 20, 2017

Contributor

I don't have a strong opinion about the change, but it would be great to get an opinion from @vit-stepanovs or @guschmue, who worked on this part of the Windows patch.

Contributor

mrry commented Mar 20, 2017

I don't have a strong opinion about the change, but it would be great to get an opinion from @vit-stepanovs or @guschmue, who worked on this part of the Windows patch.

@guschmue

This comment has been minimized.

Show comment
Hide comment
@guschmue

guschmue Mar 20, 2017

Contributor

@snnn - can you provide performance numbers before and after your change ?
Basically how much perf hit is it removing FILE_FLAG_RANDOM_ACCESS?
What at what filesize do you start seeing issues ?

Contributor

guschmue commented Mar 20, 2017

@snnn - can you provide performance numbers before and after your change ?
Basically how much perf hit is it removing FILE_FLAG_RANDOM_ACCESS?
What at what filesize do you start seeing issues ?

@snnn

This comment has been minimized.

Show comment
Hide comment
@snnn

snnn Mar 21, 2017

Contributor

Hi @guschmue , Could you tell me why do you need this flag?

Contributor

snnn commented Mar 21, 2017

Hi @guschmue , Could you tell me why do you need this flag?

@guschmue

This comment has been minimized.

Show comment
Hide comment
@guschmue

guschmue Mar 21, 2017

Contributor

Didn't say we need the flag. I think this code was used for some storage project where random access is common (aka FILE_FLAG_RANDOM_ACCESS is used to not evict behind. I don't think this is the common access pattern for RandomAccessFile in tensorflow and removing FILE_FLAG_RANDOM_ACCESS might be a good thing for sequential reads.
Since you already measure perf after, why not measure before as well to confirm that this also helps performance.

Contributor

guschmue commented Mar 21, 2017

Didn't say we need the flag. I think this code was used for some storage project where random access is common (aka FILE_FLAG_RANDOM_ACCESS is used to not evict behind. I don't think this is the common access pattern for RandomAccessFile in tensorflow and removing FILE_FLAG_RANDOM_ACCESS might be a good thing for sequential reads.
Since you already measure perf after, why not measure before as well to confirm that this also helps performance.

@snnn

This comment has been minimized.

Show comment
Hide comment
@snnn

snnn Mar 21, 2017

Contributor

Hi @guschmue

  1. FILE_FLAG_RANDOM_ACCESS flag is a hint for Cache Manager to keep mapped views(read blocks) of the file in memory as long as possible, until Memory Manager doesn’t signal low memory condition. So, if we run tensorflow standalone, without sharing resources with other programs, the performance is almost the same. However, if we run it on yarn, the situation is quite different. And it's hard to say how much it faster/slower because it number varies, and there is no way to control tensorflow's memory usage.

  2. At the same time, this flag instructs Cache Manager to disable prefetching of file data, which could reduce unnecessary disk io and increase read latency. However, we already have IoBuffer as the user mode cache. For sequential reads, I can't see any difference.

@mrry It would be great if there is a new file type like "SeqAccessFile". Actually, FILE_FLAG_RANDOM_ACCESS is good for random accesses(e.g. KV DBs, sparse model serving). It is just not good for sequential reads.

Contributor

snnn commented Mar 21, 2017

Hi @guschmue

  1. FILE_FLAG_RANDOM_ACCESS flag is a hint for Cache Manager to keep mapped views(read blocks) of the file in memory as long as possible, until Memory Manager doesn’t signal low memory condition. So, if we run tensorflow standalone, without sharing resources with other programs, the performance is almost the same. However, if we run it on yarn, the situation is quite different. And it's hard to say how much it faster/slower because it number varies, and there is no way to control tensorflow's memory usage.

  2. At the same time, this flag instructs Cache Manager to disable prefetching of file data, which could reduce unnecessary disk io and increase read latency. However, we already have IoBuffer as the user mode cache. For sequential reads, I can't see any difference.

@mrry It would be great if there is a new file type like "SeqAccessFile". Actually, FILE_FLAG_RANDOM_ACCESS is good for random accesses(e.g. KV DBs, sparse model serving). It is just not good for sequential reads.

@guschmue

This comment has been minimized.

Show comment
Hide comment
@guschmue

guschmue Mar 21, 2017

Contributor

Agree, it should be better to turn FILE_FLAG_RANDOM_ACCESS off ... will be more common to have sequential io.

Contributor

guschmue commented Mar 21, 2017

Agree, it should be better to turn FILE_FLAG_RANDOM_ACCESS off ... will be more common to have sequential io.

@martinwicke martinwicke requested a review from mrry Mar 21, 2017

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Mar 21, 2017

Contributor

I'm happy with this change if @guschmue is :). I agree that a specific sequential-access file interface would make sense for most input file processing; I think the main case for random access is when restoring a checkpoint. It's possible that (with this change) restoring a checkpoint will get slower but reading input files will get faster, and we should keep an eye on the former.

Contributor

mrry commented Mar 21, 2017

I'm happy with this change if @guschmue is :). I agree that a specific sequential-access file interface would make sense for most input file processing; I think the main case for random access is when restoring a checkpoint. It's possible that (with this change) restoring a checkpoint will get slower but reading input files will get faster, and we should keep an eye on the former.

@mrry

mrry approved these changes Mar 21, 2017

@martinwicke martinwicke merged commit 45d6994 into tensorflow:master Mar 22, 2017

12 checks passed

Android Demo App SUCCESS
Details
Linux CPU Tests SUCCESS
Details
Linux CPU Tests (Python 3) SUCCESS
Details
Linux CPU Tests CMAKE SUCCESS
Details
Linux CPU Tests Makefile SUCCESS
Details
Linux GPU SUCCESS
Details
Linux XLA SUCCESS
Details
MacOS CPU Tests SUCCESS
Details
Sanity Checks SUCCESS
Details
Windows Cmake Tests SUCCESS
Details
ci.tensorflow.org SUCCESS
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment