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

Issue 55100044: code review 55100044: runtime: use custom thunks for race calls instead of cgo (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 6 months ago by dvyukov
Modified:
7 years, 5 months ago
Reviewers:
gobot, rsc
CC:
iant, rsc, bradfitz, golang-codereviews
Visibility:
Public.

Description

runtime: use custom thunks for race calls instead of cgo Implement custom assembly thunks for hot race calls (memory accesses and function entry/exit). The thunks extract caller pc, verify that the address is in heap or global and switch to g0 stack. Before: ok regexp 3.692s ok compress/bzip2 9.461s ok encoding/json 6.380s After: ok regexp 2.229s (-40%) ok compress/bzip2 4.703s (-50%) ok encoding/json 3.629s (-43%) For comparison, normal non-race build: ok regexp 0.348s ok compress/bzip2 0.304s ok encoding/json 0.661s Race build: ok regexp 2.229s (+540%) ok compress/bzip2 4.703s (+1447%) ok encoding/json 3.629s (+449%) Also removes some race-related special cases from cgocall and scheduler. In long-term it will allow to remove cyclic runtime/race dependency on cmd/cgo. Fixes issue 4249. Fixes issue 7460. Update issue 6508 Update issue 6688

Patch Set 1 #

Patch Set 2 : diff -r 09a55add2bb5 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 3 : diff -r 09a55add2bb5 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 4 : diff -r 09a55add2bb5 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 5 : diff -r 09a55add2bb5 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 6 : diff -r 09a55add2bb5 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 7 : diff -r 09a55add2bb5 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 8 : diff -r 09a55add2bb5 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 9 : diff -r 09a55add2bb5 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 10 : diff -r d92b32d188ec https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 11 : diff -r d92b32d188ec https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 12 : diff -r efb71a1d099d https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 13 : diff -r efb71a1d099d https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 14 : diff -r d2e78612ed03 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 15 : diff -r 7473ad4d05c3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 16 : diff -r 7473ad4d05c3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 17 : diff -r 7473ad4d05c3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 18 : diff -r 7473ad4d05c3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 19 : diff -r 7473ad4d05c3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 20 : diff -r 7473ad4d05c3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 21 : diff -r 7473ad4d05c3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 22 : diff -r d067c6de4ec3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 23 : diff -r d067c6de4ec3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 24 : diff -r d067c6de4ec3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 25 : diff -r d067c6de4ec3 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 26 : diff -r ae14bde9ce3c https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 27 : diff -r ae14bde9ce3c https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 28 : diff -r ae14bde9ce3c https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 29 : diff -r ae14bde9ce3c https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 30 : diff -r ae14bde9ce3c https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 31 : diff -r ae14bde9ce3c https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 32 : diff -r ae14bde9ce3c https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 33 : diff -r 27a1bda549a2 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 34 : diff -r 27a1bda549a2 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 35 : diff -r 27a1bda549a2 https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 15

Patch Set 36 : diff -r 5f58c6d04b6d https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 37 : diff -r 32f0dc88f804 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 38 : diff -r 32f0dc88f804 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 39 : diff -r 7a45730704af https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 40 : diff -r 340da08f5f54 https://dvyukov%40google.com@code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+354 lines, -437 lines) Patch
M src/pkg/runtime/cgocall.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 3 chunks +5 lines, -22 lines 0 comments Download
M src/pkg/runtime/malloc.goc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 1 chunk +0 lines, -3 lines 0 comments Download
M src/pkg/runtime/proc.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 1 chunk +0 lines, -5 lines 0 comments Download
M src/pkg/runtime/race.h View 1 1 chunk +0 lines, -1 line 0 comments Download
M src/pkg/runtime/race.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 9 chunks +108 lines, -280 lines 0 comments Download
M src/pkg/runtime/race/README View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/runtime/race/race.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +6 lines, -112 lines 0 comments Download
M src/pkg/runtime/race/race_darwin_amd64.syso View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 Binary file 0 comments Download
M src/pkg/runtime/race/race_linux_amd64.syso View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 Binary file 0 comments Download
M src/pkg/runtime/race/race_windows_amd64.syso View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 Binary file 0 comments Download
M src/pkg/runtime/race0.c View 1 1 chunk +0 lines, -6 lines 0 comments Download
M src/pkg/runtime/race_amd64.s View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 1 chunk +234 lines, -6 lines 0 comments Download
M src/pkg/runtime/runtime.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 42
dvyukov
Hello iant@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://dvyukov%40google.com@code.google.com/p/go/
7 years, 6 months ago (2014-01-28 17:42:49 UTC) #1
dvyukov
Hi Ian, This does not work yet, and I need some help. I've changed Go->C ...
7 years, 6 months ago (2014-01-28 17:49:13 UTC) #2
dvyukov
On high-level what I want to do is: 1. Link C object file runtime/race/race_linux_amd64.syso 2. ...
7 years, 6 months ago (2014-01-28 18:13:29 UTC) #3
iant
On Tue, Jan 28, 2014 at 10:13 AM, <dvyukov@google.com> wrote: > On high-level what I ...
7 years, 6 months ago (2014-01-28 18:45:06 UTC) #4
dvyukov
On Tue, Jan 28, 2014 at 10:45 PM, Ian Lance Taylor <iant@golang.org> wrote: > On ...
7 years, 6 months ago (2014-01-28 18:48:46 UTC) #5
iant
On Tue, Jan 28, 2014 at 10:48 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > > $ ...
7 years, 6 months ago (2014-01-28 18:55:34 UTC) #6
dvyukov
On Tue, Jan 28, 2014 at 10:55 PM, Ian Lance Taylor <iant@golang.org> wrote: > On ...
7 years, 6 months ago (2014-01-28 18:58:29 UTC) #7
dvyukov
On Tue, Jan 28, 2014 at 10:58 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Tue, ...
7 years, 6 months ago (2014-01-28 18:59:37 UTC) #8
iant
On Tue, Jan 28, 2014 at 10:59 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Tue, ...
7 years, 6 months ago (2014-01-28 19:19:58 UTC) #9
dvyukov
On 2014/01/28 19:19:58, iant wrote: > On Tue, Jan 28, 2014 at 10:59 AM, Dmitry ...
7 years, 6 months ago (2014-01-29 12:44:16 UTC) #10
dvyukov
Ian, I've patched in your fix, and now cmd/cgo crashes as follows: Program received signal ...
7 years, 6 months ago (2014-01-30 15:14:55 UTC) #11
iant
On Thu, Jan 30, 2014 at 7:14 AM, <dvyukov@google.com> wrote: > > Is it possible ...
7 years, 6 months ago (2014-01-30 16:55:38 UTC) #12
dvyukov
On 2014/01/30 16:55:38, iant wrote: > On Thu, Jan 30, 2014 at 7:14 AM, <mailto:dvyukov@google.com> ...
7 years, 6 months ago (2014-01-31 09:39:01 UTC) #13
dvyukov
On 2014/01/31 09:39:01, dvyukov wrote: > On 2014/01/30 16:55:38, iant wrote: > > On Thu, ...
7 years, 6 months ago (2014-01-31 10:06:43 UTC) #14
dvyukov
On 2014/01/31 10:06:43, dvyukov wrote: > On 2014/01/31 09:39:01, dvyukov wrote: > > On 2014/01/30 ...
7 years, 6 months ago (2014-01-31 12:09:08 UTC) #15
dvyukov
OK, it seems to be working now. At least on linux. Thanks! I will update ...
7 years, 6 months ago (2014-01-31 13:44:22 UTC) #16
dvyukov
On 2014/01/31 13:44:22, dvyukov wrote: > OK, it seems to be working now. At least ...
7 years, 6 months ago (2014-02-06 11:52:31 UTC) #17
iant
On 2014/02/06 11:52:31, dvyukov wrote: > > I've done lots of refactoring and testing, and ...
7 years, 6 months ago (2014-02-06 16:41:59 UTC) #18
dvyukov
On 2014/02/06 16:41:59, iant wrote: > On 2014/02/06 11:52:31, dvyukov wrote: > > > > ...
7 years, 6 months ago (2014-02-06 16:51:54 UTC) #19
dvyukov
On 2014/02/06 16:51:54, dvyukov wrote: > On 2014/02/06 16:41:59, iant wrote: > > On 2014/02/06 ...
7 years, 6 months ago (2014-02-06 17:20:32 UTC) #20
iant
On 2014/02/06 17:20:32, dvyukov wrote: > > What relocations do I need to look for? ...
7 years, 6 months ago (2014-02-06 22:07:12 UTC) #21
iant
On 2014/02/06 16:51:54, dvyukov wrote: > On 2014/02/06 16:41:59, iant wrote: > > > The ...
7 years, 6 months ago (2014-02-06 22:21:19 UTC) #22
dvyukov
On Fri, Feb 7, 2014 at 2:21 AM, <iant@golang.org> wrote: > On 2014/02/06 16:51:54, dvyukov ...
7 years, 6 months ago (2014-02-07 05:39:21 UTC) #23
dvyukov
Hello iant@golang.org (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
7 years, 5 months ago (2014-02-24 15:01:54 UTC) #24
dvyukov
On 2014/02/06 22:21:19, iant wrote: > On 2014/02/06 16:51:54, dvyukov wrote: > > On 2014/02/06 ...
7 years, 5 months ago (2014-02-24 15:15:11 UTC) #25
iant
https://codereview.appspot.com/55100044/diff/670001/src/pkg/runtime/race.c File src/pkg/runtime/race.c (right): https://codereview.appspot.com/55100044/diff/670001/src/pkg/runtime/race.c#newcode62 src/pkg/runtime/race.c:62: // racecall allows to call an arbitrary function f ...
7 years, 5 months ago (2014-02-24 17:40:18 UTC) #26
rsc
https://codereview.appspot.com/55100044/diff/670001/src/pkg/runtime/race.c File src/pkg/runtime/race.c (right): https://codereview.appspot.com/55100044/diff/670001/src/pkg/runtime/race.c#newcode276 src/pkg/runtime/race.c:276: runtime·racesymbolize(SymbolizeContext *ctx) On 2014/02/24 17:40:19, iant wrote: > This ...
7 years, 5 months ago (2014-02-24 17:59:16 UTC) #27
iant
On Mon, Feb 24, 2014 at 9:59 AM, <rsc@golang.org> wrote: > > https://codereview.appspot.com/55100044/diff/670001/src/pkg/runtime/race.c > File ...
7 years, 5 months ago (2014-02-24 18:10:47 UTC) #28
iant
On Mon, Feb 24, 2014 at 10:10 AM, Ian Lance Taylor <iant@golang.org> wrote: > On ...
7 years, 5 months ago (2014-02-24 18:11:30 UTC) #29
rsc
I am a bit worried about all the duplicated code in race_amd64.s. It seems like ...
7 years, 5 months ago (2014-02-24 18:20:23 UTC) #30
rsc
https://codereview.appspot.com/55100044/diff/670001/src/pkg/runtime/race_amd64.s File src/pkg/runtime/race_amd64.s (right): https://codereview.appspot.com/55100044/diff/670001/src/pkg/runtime/race_amd64.s#newcode27 src/pkg/runtime/race_amd64.s:27: // They use callee-saved registers, because they are preserved ...
7 years, 5 months ago (2014-02-24 18:25:49 UTC) #31
dvyukov
https://codereview.appspot.com/55100044/diff/670001/src/pkg/runtime/race.c File src/pkg/runtime/race.c (right): https://codereview.appspot.com/55100044/diff/670001/src/pkg/runtime/race.c#newcode62 src/pkg/runtime/race.c:62: // racecall allows to call an arbitrary function f ...
7 years, 5 months ago (2014-02-25 09:24:57 UTC) #32
dvyukov
On 2014/02/24 18:20:23, rsc wrote: > I am a bit worried about all the duplicated ...
7 years, 5 months ago (2014-02-25 09:40:38 UTC) #33
rsc
Can you please do the refactoring I suggsted?
7 years, 5 months ago (2014-03-05 20:21:11 UTC) #34
dvyukov
On 2014/03/05 20:21:11, rsc wrote: > Can you please do the refactoring I suggsted? Done ...
7 years, 5 months ago (2014-03-06 14:58:30 UTC) #35
rsc
LGTM Thank you. The refactored code is much better. Why the :( ? Is it ...
7 years, 5 months ago (2014-03-06 15:35:17 UTC) #36
dvyukov
On 2014/03/06 15:35:17, rsc wrote: > LGTM > > Thank you. The refactored code is ...
7 years, 5 months ago (2014-03-06 15:46:06 UTC) #37
rsc
Okay, well we can address that in a separate CL when the time comes.
7 years, 5 months ago (2014-03-06 15:49:06 UTC) #38
bradfitz
Is CL description updated with new (12% slower) numbers? On Thu, Mar 6, 2014 at ...
7 years, 5 months ago (2014-03-06 17:14:13 UTC) #39
dvyukov
no On Thu, Mar 6, 2014 at 9:14 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Is ...
7 years, 5 months ago (2014-03-06 17:15:08 UTC) #40
dvyukov
*** Submitted as https://code.google.com/p/go/source/detail?r=6a8bd83965b3 *** runtime: use custom thunks for race calls instead of cgo ...
7 years, 5 months ago (2014-03-06 19:48:45 UTC) #41
gobot
7 years, 5 months ago (2014-03-06 21:00:27 UTC) #42
Message was sent while issue was closed.
This CL appears to have broken the linux-amd64-race builder.
Sign in to reply to this message.

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