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.

Sign up for GitHub

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

Jump to bottom

v4: all: consider io/fs changes to our APIs #630

Open
mvdan opened this issue Nov 8, 2020 · 2 comments
Open

v4: all: consider io/fs changes to our APIs #630

mvdan opened this issue Nov 8, 2020 · 2 comments
Labels
enhancement

Comments

@mvdan
Copy link
Owner

mvdan commented Nov 8, 2020

Once Go 1.17 is out, we'll be able to drop support for Go 1.15. Then we'll be able to use the new APIs in Go 1.16, such as the fs.FS interface, the faster filepath.WalkDir, and so on. At least, we should be able to make shfmt's directory walking faster (fewer stat calls), and possibly parts of expand and interp too.
mvdan added a commit that referenced this issue Sep 28, 2021
@mvdan
cmd/shfmt: use filepath.WalkDir
e380165 
And add fileutil.CouldBeScript2 to take advantage of fs.DirEntry.

Using benchcmd with "shfmt -l ." on dozens of large git repos,
we can see that avoiding stat syscalls while walking saves time:

	name           old time/op         new time/op         delta
	ShfmtGitRepos          892ms ± 2%          778ms ± 1%  -12.80%  (p=0.000 n=10+7)

	name           old user-time/op    new user-time/op    delta
	ShfmtGitRepos          699ms ±12%          628ms ±11%  -10.05%  (p=0.007 n=10+10)

	name           old sys-time/op     new sys-time/op     delta
	ShfmtGitRepos          403ms ±17%          339ms ±10%  -16.08%  (p=0.001 n=10+9)

	name           old peak-RSS-bytes  new peak-RSS-bytes  delta
	ShfmtGitRepos         33.5MB ± 5%         32.2MB ± 3%   -3.93%  (p=0.006 n=9+8)

Most end users should thus see a moderate speed-up of about 10%.

Updates #630.
@mvdan
Copy link
Owner Author

mvdan commented Sep 28, 2021

I'm leaving this issue open for v4. For example, 4c03448 left a TODO for expand.Config, since its API came before the fs package. We can't break v3 APIs, and I don't think I'm in a rush to deprecate many v3 APIs just for the sake of small performance gains.

The fileutil addition in the commit above is slightly different, because the performance does matter for shfmt.

@mvdan mvdan changed the title all: consider path/filepath and io/fs changes once we require Go 1.16 or later v4: all: consider io/fs changes to our APIs Sep 28, 2021
@alecthomas alecthomas mentioned this issue Dec 28, 2021
Merged
@mvdan
Copy link
Owner Author

mvdan commented Jan 26, 2022

Notes for future reference: we tried to add an io/fs handler to the interpreter in v3 in #799, but we gave up due to the shortcomings of the current io/fs API - it doesn't support writing files, and it doesn't support a root FS on Windows, in the sense of being able to use absolute paths with their volume names.

mvdan pushed a commit that referenced this issue May 9, 2022
@ihar-orca
interp: add custom handler for os.Stat/os.Lstat operations
af386ca 
The interpreter has interfaces for all OS interactions except stat-ing files.
Add a new handler, noting that we need both Stat and Lstat.

See #630.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@mvdan