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

Issue 97500044: code review 97500044: cmd/gc: correct handling of globals, func args, results (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 10 months ago by rsc
Modified:
10 years, 10 months ago
Reviewers:
khr
CC:
golang-codereviews, khr, iant, r
Visibility:
Public.

Description

cmd/gc: correct handling of globals, func args, results Globals, function arguments, and results are special cases in registerization. Globals must be flushed aggressively, because nearly any operation can cause a panic, and the recovery code must see the latest values. Globals also must be loaded aggressively, because nearly any store through a pointer might be updating a global: the compiler cannot see all the "address of" operations on globals, especially exported globals. To accomplish this, mark all globals as having their address taken, which effectively disables registerization. If a function contains a defer statement, the function results must be flushed aggressively, because nearly any operation can cause a panic, and the deferred code may call recover, causing the original function to return the current values of its function results. To accomplish this, mark all function results as having their address taken if the function contains any defer statements. This causes not just aggressive flushing but also aggressive loading. The aggressive loading is overkill but the best we can do in the current code. Function arguments must be considered live at all safe points in a function, because garbage collection always preserves them: they must be up-to-date in order to be preserved correctly. Accomplish this by marking them live at all call sites. An earlier attempt at this marked function arguments as having their address taken, which disabled registerization completely, making programs slower. This CL's solution allows registerization while preserving safety. The benchmark speedup is caused by being able to registerize again (the earlier CL lost the same amount). benchmark old ns/op new ns/op delta BenchmarkEqualPort32 61.4 56.0 -8.79% benchmark old MB/s new MB/s speedup BenchmarkEqualPort32 521.56 570.97 1.09x Fixes issue 1304. (again) Fixes issue 7944. (again) Fixes issue 7984. Fixes issue 7995.

Patch Set 1 #

Patch Set 2 : diff -r 712f6b41d382 https://code.google.com/p/go/ #

Patch Set 3 : diff -r b10044024a6d https://code.google.com/p/go/ #

Patch Set 4 : diff -r f099be9eddaa https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+202 lines, -180 lines) Patch
M src/cmd/5g/reg.c View 1 2 6 chunks +22 lines, -42 lines 0 comments Download
M src/cmd/6g/reg.c View 1 2 6 chunks +23 lines, -44 lines 0 comments Download
M src/cmd/8g/reg.c View 1 2 6 chunks +23 lines, -43 lines 0 comments Download
A test/fixedbugs/issue1304.go View 1 2 1 chunk +23 lines, -0 lines 0 comments Download
A test/fixedbugs/issue7995.go View 1 2 1 chunk +25 lines, -0 lines 0 comments Download
A test/fixedbugs/issue7995b.go View 1 2 1 chunk +9 lines, -0 lines 0 comments Download
A test/fixedbugs/issue7995b.dir/x1.go View 1 2 1 chunk +16 lines, -0 lines 0 comments Download
A test/fixedbugs/issue7995b.dir/x2.go View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
M test/nilptr3.go View 1 2 6 chunks +51 lines, -51 lines 0 comments Download

Messages

Total messages: 3
rsc
Hello golang-codereviews@googlegroups.com (cc: iant, khr, r), I'd like you to review this change to https://code.google.com/p/go/
10 years, 10 months ago (2014-05-15 18:07:39 UTC) #1
khr
On 2014/05/15 18:07:39, rsc wrote: > Hello mailto:golang-codereviews@googlegroups.com (cc: iant, khr, r), > > I'd ...
10 years, 10 months ago (2014-05-15 19:29:30 UTC) #2
rsc
10 years, 10 months ago (2014-05-15 19:34:55 UTC) #3
*** Submitted as https://code.google.com/p/go/source/detail?r=fc0e7019b409 ***

cmd/gc: correct handling of globals, func args, results

Globals, function arguments, and results are special cases in
registerization.

Globals must be flushed aggressively, because nearly any
operation can cause a panic, and the recovery code must see
the latest values. Globals also must be loaded aggressively,
because nearly any store through a pointer might be updating a
global: the compiler cannot see all the "address of"
operations on globals, especially exported globals. To
accomplish this, mark all globals as having their address
taken, which effectively disables registerization.

If a function contains a defer statement, the function results
must be flushed aggressively, because nearly any operation can
cause a panic, and the deferred code may call recover, causing
the original function to return the current values of its
function results. To accomplish this, mark all function
results as having their address taken if the function contains
any defer statements. This causes not just aggressive flushing
but also aggressive loading. The aggressive loading is
overkill but the best we can do in the current code.

Function arguments must be considered live at all safe points
in a function, because garbage collection always preserves
them: they must be up-to-date in order to be preserved
correctly. Accomplish this by marking them live at all call
sites. An earlier attempt at this marked function arguments as
having their address taken, which disabled registerization
completely, making programs slower. This CL's solution allows
registerization while preserving safety. The benchmark speedup
is caused by being able to registerize again (the earlier CL
lost the same amount).

benchmark                old ns/op     new ns/op     delta
BenchmarkEqualPort32     61.4          56.0          -8.79%

benchmark                old MB/s     new MB/s     speedup
BenchmarkEqualPort32     521.56       570.97       1.09x

Fixes issue 1304. (again)
Fixes issue 7944. (again)
Fixes issue 7984.
Fixes issue 7995.

LGTM=khr
R=golang-codereviews, khr
CC=golang-codereviews, iant, r
https://codereview.appspot.com/97500044
Sign in to reply to this message.

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