Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(216)

Issue 3050041: code review 3050041: go/ast: use token.Pos instead of token.Position; adjus... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 4 months ago by gri
Modified:
14 years, 3 months ago
Reviewers:
CC:
rsc, r, golang-dev
Visibility:
Public.

Description

go/ast: use token.Pos instead of token.Position; adjust all dependent code Specifically: * lib/godoc: - provide file set (FSet) argument to formatters where needed * src/cmd: - cgo, ebnflint, godoc, gofmt, goinstall: provide file set (fset) where needed - godoc: remove local binary search with sort.Search (change by rsc), extract file set for formatters * src/pkg: - exp/eval: remove embedded token.Position fields from nodes and replace with named token.Pos fields; add corresponding Pos() accessor methods - go/token: added file.Line(), changed signature of File.Position() * test/fixedbugs/: - bug206.go: change test to not rely on token.Pos details * added various extra comments * Runs all.bash * gofmt formats all of src, misc w/o changes * godoc runs * performance: - The new version of godoc consumes about the same space after indexing has completed, but indexing is half the speed. Significant space savings are expected from smaller ASTs, but since they are thrown away after a file has been indexed, this is not visible anymore. The slower indexing time is due to the much more expensive computation of line information. However, with the new compressed position information, indexing can be rewritten and simplified. Furthermore, computing the line info can be done more efficiently. New godoc, immediately after indexing completed (best of three runs): PID COMMAND %CPU TIME #TH #PRTS #MREGS RPRVT RSHRD RSIZE VSIZE 44381 godoc 0.0% 0:38.00 4 19 149 145M 184K 148M 176M 2010/12/03 17:58:35 index updated (39.231s, 18505 unique words, 386387 spots) 2010/12/03 17:58:35 bytes=90858456 footprint=199182584 2010/12/03 17:58:36 bytes=47858568 footprint=167295224 Old godoc, immediately after indexing completed (best of three runs): PID COMMAND %CPU TIME #TH #PRTS #MREGS RPRVT RSHRD RSIZE VSIZE 23167 godoc 0.0% 0:22.02 4 17 132 129M 184K 132M 173M 2010/12/03 14:51:32 index updated (24.892s, 18765 unique words, 393830 spots) 2010/12/03 14:51:32 bytes=66404528 footprint=163907832 2010/12/03 14:51:32 bytes=46282224 footprint=163907832 The different numbers for unique words/spots stem from the fact the the two workspaces are not exactly identical. The new godoc maintains a large file set data structure during indexing which (probably) is the reason for the larger heap (90858456 vs 66404528) before garbage collection.

Patch Set 1 #

Patch Set 2 : code review 3050041: adjust code to use new token.Pos position encoding #

Patch Set 3 : code review 3050041: use token.Pos instead of token.Position #

Patch Set 4 : code review 3050041: use token.Pos instead of token.Position #

Patch Set 5 : code review 3050041: use token.Pos instead of token.Position #

Patch Set 6 : code review 3050041: use token.Pos instead of token.Position #

Patch Set 7 : code review 3050041: use token.Pos instead of token.Position #

Patch Set 8 : code review 3050041: use token.Pos instead of token.Position #

Patch Set 9 : code review 3050041: use token.Pos instead of token.Position #

Patch Set 10 : code review 3050041: go/ast: use token.Pos instead of token.Position; adjus... #

Patch Set 11 : code review 3050041: go/ast: use token.Pos instead of token.Position; adjus... #

Total comments: 3

Patch Set 12 : code review 3050041: go/ast: use token.Pos instead of token.Position; adjus... #

Patch Set 13 : code review 3050041: go/ast: use token.Pos instead of token.Position; adjus... #

Patch Set 14 : code review 3050041: go/ast: use token.Pos instead of token.Position; adjus... #

Patch Set 15 : code review 3050041: go/ast: use token.Pos instead of token.Position; adjus... #

Patch Set 16 : code review 3050041: go/ast: use token.Pos instead of token.Position; adjus... #

Patch Set 17 : code review 3050041: go/ast: use token.Pos instead of token.Position; adjus... #

Unified diffs Side-by-side diffs Delta from patch set Stats (+782 lines, -691 lines) Patch
M lib/godoc/package.html View 6 7 3 chunks +20 lines, -14 lines 0 comments Download
M lib/godoc/package.txt View 4 chunks +8 lines, -8 lines 0 comments Download
M src/cmd/cgo/ast.go View 3 chunks +4 lines, -3 lines 0 comments Download
M src/cmd/cgo/gcc.go View 3 chunks +3 lines, -3 lines 0 comments Download
M src/cmd/cgo/main.go View 5 chunks +6 lines, -4 lines 0 comments Download
M src/cmd/cgo/out.go View 1 2 3 6 chunks +7 lines, -7 lines 0 comments Download
M src/cmd/cgo/util.go View 1 2 3 4 5 6 1 chunk +2 lines, -3 lines 0 comments Download
M src/cmd/ebnflint/ebnflint.go View 1 2 chunks +4 lines, -2 lines 0 comments Download
M src/cmd/godoc/dirtrees.go View 1 2 3 5 chunks +7 lines, -4 lines 0 comments Download
M src/cmd/godoc/godoc.go View 1 2 3 4 5 6 14 15 20 chunks +42 lines, -47 lines 0 comments Download
M src/cmd/godoc/index.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +12 lines, -7 lines 0 comments Download
M src/cmd/godoc/main.go View 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/godoc/snippet.go View 1 2 3 4 5 6 chunks +12 lines, -11 lines 0 comments Download
M src/cmd/godoc/spec.go View 1 2 3 4 5 6 7 6 chunks +18 lines, -15 lines 0 comments Download
M src/cmd/gofmt/gofmt.go View 4 chunks +4 lines, -2 lines 0 comments Download
M src/cmd/gofmt/rewrite.go View 2 chunks +2 lines, -2 lines 0 comments Download
M src/cmd/goinstall/main.go View 2 chunks +2 lines, -0 lines 0 comments Download
M src/cmd/goinstall/parse.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/ebnf/ebnf.go View 1 2 3 4 5 6 12 chunks +40 lines, -35 lines 0 comments Download
M src/pkg/ebnf/ebnf_test.go View 1 2 chunks +6 lines, -2 lines 0 comments Download
M src/pkg/ebnf/parser.go View 1 2 3 4 5 6 6 chunks +21 lines, -13 lines 0 comments Download
M src/pkg/exp/datafmt/datafmt_test.go View 1 chunk +5 lines, -1 line 0 comments Download
M src/pkg/exp/datafmt/parser.go View 1 2 3 4 5 6 7 8 7 chunks +22 lines, -16 lines 0 comments Download
M src/pkg/exp/eval/bridge.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/exp/eval/compiler.go View 1 2 chunks +5 lines, -9 lines 0 comments Download
M src/pkg/exp/eval/eval_test.go View 3 chunks +5 lines, -1 line 0 comments Download
M src/pkg/exp/eval/expr.go View 1 10 chunks +12 lines, -12 lines 0 comments Download
M src/pkg/exp/eval/main.go View 5 chunks +7 lines, -5 lines 0 comments Download
M src/pkg/exp/eval/scope.go View 5 chunks +18 lines, -10 lines 0 comments Download
M src/pkg/exp/eval/stmt.go View 1 13 chunks +19 lines, -22 lines 0 comments Download
M src/pkg/exp/eval/type.go View 1 5 chunks +10 lines, -6 lines 0 comments Download
M src/pkg/exp/eval/typec.go View 1 14 chunks +22 lines, -22 lines 0 comments Download
M src/pkg/exp/eval/world.go View 6 chunks +18 lines, -15 lines 0 comments Download
M src/pkg/exp/ogle/cmd.go View 4 chunks +4 lines, -4 lines 0 comments Download
M src/pkg/go/ast/ast.go View 1 24 chunks +148 lines, -148 lines 0 comments Download
M src/pkg/go/ast/filter.go View 1 2 3 4 5 6 7 3 chunks +6 lines, -27 lines 0 comments Download
M src/pkg/go/doc/doc.go View 3 chunks +2 lines, -3 lines 0 comments Download
M src/pkg/go/parser/interface.go View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +23 lines, -18 lines 0 comments Download
M src/pkg/go/parser/parser.go View 1 2 3 4 5 6 7 20 chunks +37 lines, -30 lines 0 comments Download
M src/pkg/go/parser/parser_test.go View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +7 lines, -4 lines 0 comments Download
M src/pkg/go/printer/nodes.go View 1 2 3 4 5 29 chunks +35 lines, -32 lines 0 comments Download
M src/pkg/go/printer/printer.go View 1 2 3 4 5 6 10 chunks +15 lines, -12 lines 0 comments Download
M src/pkg/go/printer/printer_test.go View 4 chunks +6 lines, -2 lines 0 comments Download
M src/pkg/go/scanner/scanner.go View 1 2 3 4 5 6 7 25 chunks +58 lines, -67 lines 0 comments Download
M src/pkg/go/scanner/scanner_test.go View 1 2 3 4 5 6 7 8 11 chunks +30 lines, -17 lines 0 comments Download
M src/pkg/go/token/position.go View 8 9 10 11 12 3 chunks +24 lines, -7 lines 0 comments Download
M src/pkg/go/token/position_test.go View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/go/typechecker/scope.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/go/typechecker/typechecker.go View 1 5 chunks +9 lines, -6 lines 0 comments Download
M src/pkg/go/typechecker/typechecker_test.go View 1 5 chunks +7 lines, -5 lines 0 comments Download
M test/fixedbugs/bug206.go View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 7
gri
Hello rsc, r (cc: golang-dev@googlegroups.com), I'd like you to review this change.
14 years, 3 months ago (2010-12-03 23:13:52 UTC) #1
gri
Hello rsc, r (cc: golang-dev@googlegroups.com), I'd like you to review this change.
14 years, 3 months ago (2010-12-03 23:14:23 UTC) #2
rsc1
LGTM http://codereview.appspot.com/3050041/diff/43001/src/pkg/go/parser/interface.go File src/pkg/go/parser/interface.go (right): http://codereview.appspot.com/3050041/diff/43001/src/pkg/go/parser/interface.go#newcode126 src/pkg/go/parser/interface.go:126: // optional parser functionality. Position information is recorded ...
14 years, 3 months ago (2010-12-03 23:44:18 UTC) #3
gri
There is something not quite right: The file set used for creating the index should ...
14 years, 3 months ago (2010-12-03 23:50:09 UTC) #4
gri
Hello rsc, r (cc: golang-dev@googlegroups.com), I'd like you to review this change.
14 years, 3 months ago (2010-12-04 02:03:46 UTC) #5
rsc
LGTM
14 years, 3 months ago (2010-12-04 02:11:22 UTC) #6
gri
14 years, 3 months ago (2010-12-06 22:23:21 UTC) #7
*** Submitted as http://code.google.com/p/go/source/detail?r=545c9926d61a ***

	go/ast: use token.Pos instead of token.Position; adjust all dependent code

	Specifically:

	* lib/godoc:
	- provide file set (FSet) argument to formatters where needed

	* src/cmd:
	- cgo, ebnflint, godoc, gofmt, goinstall: provide file set (fset) where needed
	- godoc: remove local binary search with sort.Search (change by rsc),
	  extract file set for formatters

	* src/pkg:
	- exp/eval: remove embedded token.Position fields from nodes and replace
	  with named token.Pos fields; add corresponding Pos() accessor methods
	- go/token: added file.Line(), changed signature of File.Position()

	* test/fixedbugs/:
	- bug206.go: change test to not rely on token.Pos details

	* added various extra comments
	* Runs all.bash
	* gofmt formats all of src, misc w/o changes
	* godoc runs

	* performance:
	- The new version of godoc consumes about the same space after indexing
	  has completed, but indexing is half the speed. Significant space savings
	  are expected from smaller ASTs, but since they are thrown away after a
	  file has been indexed, this is not visible anymore. The slower indexing
	  time is due to the much more expensive computation of line information.
	  However, with the new compressed position information, indexing can be
	  rewritten and simplified. Furthermore, computing the line info can be
	  done more efficiently.

        New godoc, immediately after indexing completed (best of three runs):

	  PID COMMAND      %CPU   TIME   #TH #PRTS #MREGS RPRVT  RSHRD  RSIZE  VSIZE
	44381 godoc        0.0%  0:38.00   4    19    149  145M   184K   148M   176M

	2010/12/03 17:58:35 index updated (39.231s, 18505 unique words, 386387 spots)
	2010/12/03 17:58:35 bytes=90858456 footprint=199182584
	2010/12/03 17:58:36 bytes=47858568 footprint=167295224

	Old godoc, immediately after indexing completed (best of three runs):

	  PID COMMAND      %CPU   TIME   #TH #PRTS #MREGS RPRVT  RSHRD  RSIZE  VSIZE
	23167 godoc        0.0%  0:22.02   4    17    132  129M   184K   132M   173M

	2010/12/03 14:51:32 index updated (24.892s, 18765 unique words, 393830 spots)
	2010/12/03 14:51:32 bytes=66404528 footprint=163907832
	2010/12/03 14:51:32 bytes=46282224 footprint=163907832

	The different numbers for unique words/spots stem from the fact the the
	two workspaces are not exactly identical. The new godoc maintains a large
	file set data structure during indexing which (probably) is the reason
	for the larger heap (90858456 vs 66404528) before garbage collection.

R=rsc, r
CC=golang-dev
http://codereview.appspot.com/3050041
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b