Chromium Code Reviews

Issue 6647012: Fix memory leaks on x64... (Closed)

Created:
9 years, 9 months ago by Rico
Modified:
9 years, 6 months ago
Reviewers:
Lasse Reichstein, Kevin Millikin (Chromium)
CC:
v8-dev
Visibility:
Public.

Description

Fix memory leaks on x64 This change uses ZoneObject as base class for our jumptable entry. In addition this change refactors the JumpTableEntry a bit. Committed: http://code.google.com/p/v8/source/detail?r=7095

Patch Set 1 #

Total comments: 2

Patch Set 2 : '' #

Total comments: 5

Patch Set 3 : '' #

Total comments: 2
Unified diffs Side-by-side diffs Stats (+6 lines, -10 lines)
M src/x64/lithium-codegen-x64.h View 1 chunk +1 line, -1 line 1 comment
M src/x64/lithium-codegen-x64.cc View 2 chunks +5 lines, -9 lines 1 comment

Messages

Total messages: 9 (0 generated)
Rico
9 years, 9 months ago (2011-03-09 08:54:21 UTC) #1
Kevin Millikin (Chromium)
Drive by: is there any reason to malloc these at all (zone or otherwise)? Just ...
9 years, 9 months ago (2011-03-09 09:02:10 UTC) #2
Lasse Reichstein
If the ZoneList grows, it will copy the contents to a new and longer backing ...
9 years, 9 months ago (2011-03-09 09:09:17 UTC) #3
Lasse Reichstein
Can you explain why this changes the memory behavior of the code? I can't see ...
9 years, 9 months ago (2011-03-09 09:16:54 UTC) #4
Lasse Reichstein
Never mind me. I missed that the new class was a ZoneObject. I still think ...
9 years, 9 months ago (2011-03-09 09:36:12 UTC) #5
Rico
Changed this to use Kevins suggestion.
9 years, 9 months ago (2011-03-09 09:48:06 UTC) #6
Kevin Millikin (Chromium)
LGTM if comments below are addressed. http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64.cc File src/x64/lithium-codegen-x64.cc (right): http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64.cc#newcode261 src/x64/lithium-codegen-x64.cc:261: JumpTableEntry* info = ...
9 years, 9 months ago (2011-03-09 09:54:32 UTC) #7
Rico
Added usage of last() and is_empty() on the zonelist instead of using the length.
9 years, 9 months ago (2011-03-09 09:55:44 UTC) #8
Lasse Reichstein
9 years, 9 months ago (2011-03-09 09:57:06 UTC) #9
LGTM.

http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:261: JumpTableEntry* info = &jump_table_[i];
Agree.

http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64.h
File src/x64/lithium-codegen-x64.h (right):

http://codereview.chromium.org/6647012/diff/5001/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.h:247: Label label_;
Kevin also reminded me that struct fields don't need the "_" suffix, so while
you're at it ... :)

http://codereview.chromium.org/6647012/diff/5003/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/6647012/diff/5003/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:545: jump_table_.Add(entry);
If the JumpTableEntry constructor was explicit, this wouldn't typecheck. You'll
need .Add(JumpTableEntry(address))

http://codereview.chromium.org/6647012/diff/5003/src/x64/lithium-codegen-x64.h
File src/x64/lithium-codegen-x64.h (right):

http://codereview.chromium.org/6647012/diff/5003/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.h:244: inline JumpTableEntry(Address address)
Whoops, we forgot the "explicit" here. I'm surprised it lints.

Powered by Google App Engine