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

Issue 6650054: code review 6650054: cmd/gc: use bit-fields for variables in struct Node (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 5 months ago by atom
Modified:
11 years, 5 months ago
Reviewers:
minux1, rsc
CC:
golang-dev
Visibility:
Public.

Description

cmd/gc: use bit-fields for variables in struct Node This changes sizeof(Node) from 212 to 204 bytes on i386.

Patch Set 1 #

Total comments: 1

Patch Set 2 : diff -r cfa9208b98fc https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -8 lines) Patch
M src/cmd/gc/go.h View 1 1 chunk +7 lines, -7 lines 0 comments Download
M src/cmd/gc/walk.c View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 6
atom
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
11 years, 5 months ago (2012-10-12 16:59:44 UTC) #1
minux1
https://codereview.appspot.com/6650054/diff/1/src/cmd/gc/go.h File src/cmd/gc/go.h (right): https://codereview.appspot.com/6650054/diff/1/src/cmd/gc/go.h#newcode260 src/cmd/gc/go.h:260: schar likely; // likeliness of if statement i think ...
11 years, 5 months ago (2012-10-12 17:08:44 UTC) #2
atom
Hello golang-dev@googlegroups.com, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 5 months ago (2012-10-12 17:51:19 UTC) #3
minux1
LGTM. Please wait for rsc to sign off.
11 years, 5 months ago (2012-10-12 18:04:57 UTC) #4
rsc
I'm sorry, but we're not going to use bitfields here. They have a long history ...
11 years, 5 months ago (2012-10-12 18:35:23 UTC) #5
atom
11 years, 5 months ago (2012-10-12 20:27:11 UTC) #6
On 2012/10/12 18:35:23, rsc wrote:
> I'm sorry, but we're not going to use bitfields here. They have a long history
> of problems and complications, and I'd prefer that we keep them out of this
code
> base.

The bitfields were part of CL 6345044. I will delete them from 6345044.
 
> I have no problem collapsing the fields into a uint64 flag or something like
> that, which can be tested with if(n->flag & Whatever) etc.
> 
> I'm a little surprised that these 6 bytes are the main memory overhead.

It is true that there exist larger memory overheads.

> Removing
> even a single pointer field, such as by making Val val into Val *val,
allocated
> as needed, seems like it would have a larger effect.

sizeof(Val) is just 2 pointers, so the effect of replacing it with Val* is of
the same order as the effect of collapsing the 6 bytes in this CL.

The code in src/cmd/gc/walk.c seems incorrect (uchar may overflow?), so at the
very least we could merge src/cmd/gc/walk.c

> I think there's interesting work to be done here but I would prefer it to be
> driven by profiling data.
Sign in to reply to this message.

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