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

Issue 83740044: code review 83740044: cmd/gc, runtime: optimize map[string] lookup from []byte key (Closed)

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

Description

cmd/gc, runtime: optimize map[string] lookup from []byte key Brad has been asking for this for a while. I have resisted because I wanted to find a more general way to do this, one that would keep the performance of code introducing variables the same as the performance of code that did not. (See golang.org/issue/3512#c20). I have not found the more general way, and recent changes to remove ambiguously live temporaries have blown away the property I was trying to preserve, so that's no longer a reason not to make the change. Fixes issue 3512.

Patch Set 1 #

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+85 lines, -3 lines) Patch
M src/cmd/gc/builtin.c View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/gc/go.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/gc/order.c View 1 2 2 chunks +21 lines, -3 lines 0 comments Download
M src/cmd/gc/runtime.go View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/gc/walk.c View 1 1 chunk +5 lines, -0 lines 0 comments Download
M src/pkg/runtime/map_test.go View 1 2 1 chunk +37 lines, -0 lines 0 comments Download
M src/pkg/runtime/string.goc View 1 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 9
rsc
Hello iant (cc: bradfitz, golang-codereviews@googlegroups.com, khr, r), I'd like you to review this change to ...
5 years, 10 months ago (2014-04-03 03:26:18 UTC) #1
bradfitz
I got unexpected results trying to use this in an experiment just now, in net/textproto's ...
5 years, 10 months ago (2014-04-03 17:39:33 UTC) #2
rsc
Run the malloc profiler to find where the allocations are?
5 years, 10 months ago (2014-04-03 18:02:14 UTC) #3
iant
LGTM
5 years, 10 months ago (2014-04-03 20:53:57 UTC) #4
bradfitz
From chat with Russ, we determined that the optimization works for mapaccess1 but not mapaccess2: ...
5 years, 10 months ago (2014-04-03 21:19:02 UTC) #5
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=881e5ceda5bf *** cmd/gc, runtime: optimize map[string] lookup from []byte key Brad has ...
5 years, 10 months ago (2014-04-03 23:05:23 UTC) #6
oleku
Would this be extended to mapaccess2 ? On 2014/04/03 23:05:23, rsc wrote: > *** Submitted ...
5 years, 9 months ago (2014-04-08 00:57:58 UTC) #7
bradfitz
It was. On Apr 7, 2014 5:57 PM, <oleku.konko@gmail.com> wrote: > Would this be extended ...
5 years, 9 months ago (2014-04-08 01:01:48 UTC) #8
oleku
5 years, 9 months ago (2014-04-08 01:40:37 UTC) #9
Message was sent while issue was closed.
Thanks for the clarification. Well done.

On 2014/04/08 01:01:48, bradfitz wrote:
> It was.
>  On Apr 7, 2014 5:57 PM, <mailto:oleku.konko@gmail.com> wrote:
> 
> > Would this be extended to mapaccess2 ?
> >
> >
> > On 2014/04/03 23:05:23, rsc wrote:
> >
> >> *** Submitted as
> >>
> > https://code.google.com/p/go/source/detail?r=881e5ceda5bf ***
> >
> >  cmd/gc, runtime: optimize map[string] lookup from []byte key
> >>
> >
> >  Brad has been asking for this for a while.
> >> I have resisted because I wanted to find a more general way to
> >> do this, one that would keep the performance of code introducing
> >> variables the same as the performance of code that did not.
> >> (See golang.org/issue/3512#c20).
> >>
> >
> >  I have not found the more general way, and recent changes to
> >> remove ambiguously live temporaries have blown away the
> >> property I was trying to preserve, so that's no longer a reason
> >> not to make the change.
> >>
> >
> >  Fixes issue 3512.
> >>
> >
> >  LGTM=iant
> >> R=iant
> >> CC=bradfitz, golang-codereviews, khr, r
> >> https://codereview.appspot.com/83740044
> >>
> >
> >
> > https://codereview.appspot.com/83740044/
> >
Sign in to reply to this message.

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