Skip to content
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

os: Open() makes two system call to open a file on windows #7426

Closed
pmezard opened this issue Feb 27, 2014 · 8 comments
Closed

os: Open() makes two system call to open a file on windows #7426

pmezard opened this issue Feb 27, 2014 · 8 comments
Milestone

Comments

@pmezard
Copy link
Contributor

@pmezard pmezard commented Feb 27, 2014

What steps will reproduce the problem?

1. Write a simple program calling os.Open and exiting. Run it on Windows while
monitoring it with procmon (or alternatively, read the code).

What is the expected output?

A single CreateFile system call.

What do you see instead?

Two CreateFile calls.

Which operating system are you using?

Windows 7

Which version are you using?  (run 'go version' or 'gccgo --version')

changeset:   19292:65546d299e5a
user:        Josh Bleecher Snyder <josharian@gmail.com>
date:        Wed Feb 26 12:25:13 2014 -0800
summary:     cmd/gc: fix bad checknil with ints on 32 bit compilers

Please provide any additional information below.

This is caused by Open() in file_windows.go calling openDir() first then openFile().
There is a brainman comment saying:

  // TODO(brainman): not sure about my logic of assuming it is dir first, then fall back to file

There is no way to avoid 2 system calls in all cases, but I think opening a file should
be tried before opening a directory. I do not recall having used the latter (but maybe
it is used internally). I have no concrete problem or benchmark about this.

Switching the calls, and returning the file error when both openFile and openDir fail
fixes the issue and pass all tests. I do not think openFile might succeed where openDir
did before. Reading:

  http://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx

The only place mentioning directory handle is the dwFlagsAndAttributes section, about
FILE_FLAG_BACKUP_SEMANTICS, which is never used by syscall.Open().

 I can submit a CL if you want.
@robpike
Copy link
Contributor

@robpike robpike commented Feb 27, 2014

Comment 1:

Labels changed: added repo-main, release-go1.3maybe, os-windows.

Owner changed to @alexbrainman.

Status changed to Accepted.

@robpike
Copy link
Contributor

@robpike robpike commented Feb 27, 2014

Comment 2:

Marked 1.3Maybe because it looks needless but safe and easy to fix.
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Feb 28, 2014

Comment 3:

patrick,
It sounds reasonable to change, but can you actually demonstrate your point with some
statistical data.
Can you change some large program that uses many files (cmd/go or godoc or ...) to count
"failed syscalls" for both scenarios (open file first against open directory first), and
tell us what the difference is. You should consider syscall is "failed" if openDir
failed while openFile succeeds.
Thank you
Alex
@pmezard
Copy link
Contributor Author

@pmezard pmezard commented Mar 1, 2014

Comment 4:

I added Stderr.Write([]byte("...")) before every return and between the two calls. Then
built everything again and ran "go install -a std" a couple of times for both
configurations, piping to "sort | uniq -c". The results, reordered by code flow.
Before:
-----
547   openDir succeeded
3593 openDir failed and fell back to openFile
3592 openFile succeeded
1       both failed
After:
----
3592 openFile succeeded
548   openFile failed and fell back
547   openDir succeeded
1       both failed
So, it trades 3593 failed openDir for 548 failed openFile.
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Mar 2, 2014

Comment 5:

Patrick,
Looks good to me. Please send your change as described here
http://golang.org/doc/contribute.html#Code_review. Thank you.
Alex
@pmezard
Copy link
Contributor Author

@pmezard pmezard commented Mar 2, 2014

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Mar 4, 2014

Comment 7:

Status changed to Started.

@rsc
Copy link
Contributor

@rsc rsc commented Apr 3, 2014

Comment 8:

The CL said "Fix issue" instead of "Fixes issue".

Status changed to Fixed.

@pmezard pmezard added fixed labels Apr 3, 2014
@rsc rsc added this to the Go1.3 milestone Apr 14, 2015
@rsc rsc removed the release-go1.3maybe label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.