Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1002)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1838973005: Subzero. Liveness memory management. (Closed)

Created:
4 years, 8 months ago by John
Modified:
4 years, 8 months ago
CC:
native-client-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Patch Set 1 : Templatizes BitVector. #

Patch Set 2 : Uses a Liveness-local allocator. #

Patch Set 3 : Adds named ctor to Ice::Liveness #

Patch Set 4 : Removes left over calls to dumpStats #

Patch Set 5 : Adds the LivenessAllocatorScopeInitialization -- it was removed erroneously. #

Patch Set 6 : Make format #

Patch Set 7 : Some tweaks to the LivenessAllocatorScope management. #

Patch Set 8 : Adds TODO to investigate why CfgLocalAllocators can't be cached #

Total comments: 8

Patch Set 9 : Addresses comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+154 lines, -54 lines) Patch
M src/IceBitVector.h View 1 2 3 4 5 6 7 8 23 chunks +44 lines, -39 lines 0 comments Download
M src/IceCfg.cpp View 1 2 3 4 5 6 2 chunks +9 lines, -1 line 0 comments Download
M src/IceDefs.h View 1 2 3 4 5 2 chunks +5 lines, -3 lines 0 comments Download
M src/IceGlobalContext.cpp View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -0 lines 0 comments Download
M src/IceLiveness.h View 1 2 3 4 5 6 7 8 4 chunks +36 lines, -7 lines 0 comments Download
M src/IceLiveness.cpp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M src/IceMemory.h View 1 2 3 4 5 6 7 6 chunks +44 lines, -3 lines 0 comments Download
M src/IceMemory.cpp View 1 2 3 4 5 2 chunks +13 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (2 generated)
John
presubmit happy, non-performance killing patch.
4 years, 8 months ago (2016-03-31 14:20:59 UTC) #2
Jim Stichnoth
lgtm https://codereview.chromium.org/1838973005/diff/140001/src/IceBitVector.h File src/IceBitVector.h (right): https://codereview.chromium.org/1838973005/diff/140001/src/IceBitVector.h#newcode659 src/IceBitVector.h:659: // following assert, we make sure BitVectorTmpls grow ...
4 years, 8 months ago (2016-03-31 16:23:20 UTC) #3
John
Committed patchset #9 (id:160001) manually as 7bb9cab327047766b9fb4617475a50cd628abee9 (presubmit successful).
4 years, 8 months ago (2016-04-01 12:43:14 UTC) #5
John
4 years, 8 months ago (2016-04-01 13:52:03 UTC) #6
Message was sent while issue was closed. 
=

https://codereview.chromium.org/1838973005/diff/140001/src/IceBitVector.h
File src/IceBitVector.h (right):

https://codereview.chromium.org/1838973005/diff/140001/src/IceBitVector.h#new...
src/IceBitVector.h:659: // following assert, we make sure BitVectorTmpls grow in
a single step from
On 2016/03/31 16:23:20, stichnot wrote:
> reflow comment

Done.

https://codereview.chromium.org/1838973005/diff/140001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1838973005/diff/140001/src/IceGlobalContext.c...
src/IceGlobalContext.cpp:25: #include "IceLiveness.h"
On 2016/03/31 16:23:20, stichnot wrote:
> Alphabetize?

I guess jpp::less<Alphabet> had a bug? :P

Done.

https://codereview.chromium.org/1838973005/diff/140001/src/IceLiveness.h
File src/IceLiveness.h (right):

https://codereview.chromium.org/1838973005/diff/140001/src/IceLiveness.h#newc...
src/IceLiveness.h:119: SizeT MaxLocals = 0;
On 2016/03/31 16:23:20, stichnot wrote:
> Should these data members be grouped with the other private data members
below?

Done.

https://codereview.chromium.org/1838973005/diff/140001/src/IceLiveness.h#newc...
src/IceLiveness.h:130: assert(false);
On 2016/03/31 16:23:20, stichnot wrote:
> I like this very much.  What do you think about renaming this function to
> something like checkSize(), with a report_fatal_error()?

I would expect the code to work correctly if the array needs to be resized. It
is just that we do not expect the resize to be needed. That's why I went with
the assert in lieu of the fatal error. I left as is, but no strong opinions what
so ever -- I can change if you want.

Powered by Google App Engine
This is Rietveld 408576698