Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(482)

Issue 2698673003: Call HeapObjectHeader::checkHeader solely for its side-effect. (Closed)

Created:
10 months ago by palmer
Modified:
8 months, 2 weeks ago
Reviewers:
haraken, sof, sigbjornf
CC:
Mads Ager (chromium), blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews, kinuko+watch, kouhei+heap_chromium.org, oilpan-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Call HeapObjectHeader::checkHeader solely for its side-effect. This requires changing its signature. This is a preliminary stage to making it private. BUG=633030 Review-Url: https://codereview.chromium.org/2698673003 Cr-Commit-Position: refs/heads/master@{#460489} Committed: https://chromium.googlesource.com/chromium/src/+/0749ec24fae74ec32d0567eef0e5ec43c84dbcb9

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fix another call site, and use CHECK_EQ instead of DCHECK. #

Patch Set 3 : Rebase and use DCHECK_EQ. #

Patch Set 4 : Rebase. #

Patch Set 5 : Rebase. #

Patch Set 6 : Use DCHECK on a bool instead of DCHECK_EQ. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -33 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h View 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/heap/GCInfo.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/platform/heap/HeapAllocator.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/heap/HeapAllocator.cpp View 3 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/platform/heap/HeapPage.h View 1 2 3 4 5 4 chunks +11 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/platform/heap/HeapPage.cpp View 1 2 3 4 9 chunks +9 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/platform/heap/Member.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/platform/heap/TraceTraits.h View 1 2 3 4 4 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 54 (37 generated)
palmer
10 months ago (2017-02-16 17:28:33 UTC) #2
sof
it looks like you haven't converted all occurrences of checkHeader() ? (I don't understand how ...
10 months ago (2017-02-16 18:50:04 UTC) #8
jbroman
one drive-by comment (just saw "DCHECK" and "==" on the same line of code) https://codereview.chromium.org/2698673003/diff/1/third_party/WebKit/Source/platform/heap/HeapPage.h ...
10 months ago (2017-02-16 19:08:10 UTC) #9
palmer
> it looks like you haven't converted all occurrences of checkHeader() ? Fixed. > (I ...
10 months ago (2017-02-16 19:45:19 UTC) #10
sof
On 2017/02/16 19:45:19, palmer wrote: > > it looks like you haven't converted all occurrences ...
10 months ago (2017-02-16 21:21:13 UTC) #15
palmer
> I'd like to see some perf numbers on that first, so go back to ...
10 months ago (2017-02-16 21:59:45 UTC) #16
haraken
In theory (=if the compiler is smart), this should not regress performance, right? LGTM if ...
10 months ago (2017-02-16 23:42:48 UTC) #17
palmer
> In theory (=if the compiler is smart), this should not regress performance, right? I ...
10 months ago (2017-02-17 00:37:13 UTC) #18
sof
On 2017/02/16 23:42:48, haraken wrote: > In theory (=if the compiler is smart), this should ...
10 months ago (2017-02-17 07:08:06 UTC) #19
palmer
> > In theory (=if the compiler is smart), this should not regress performance, > ...
9 months, 2 weeks ago (2017-02-28 00:11:43 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698673003/60001
9 months, 2 weeks ago (2017-02-28 00:12:31 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/319231)
9 months, 2 weeks ago (2017-02-28 04:39:52 UTC) #25
palmer
> Try jobs failed on following builders: > linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/319231) haraken: I ...
8 months, 2 weeks ago (2017-03-28 19:22:06 UTC) #34
sof
On 2017/03/28 19:22:06, palmer wrote: > > Try jobs failed on following builders: > > ...
8 months, 2 weeks ago (2017-03-28 19:43:23 UTC) #35
palmer
> NO_SANITIZE_ADDRESS (and other NO_SANITIZEs) only applies to the entry point it annotates, i.e., all ...
8 months, 2 weeks ago (2017-03-28 21:17:29 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698673003/100001
8 months, 2 weeks ago (2017-03-29 16:56:20 UTC) #51
commit-bot: I haz the power
8 months, 2 weeks ago (2017-03-29 19:29:48 UTC) #54
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0749ec24fae74ec32d0567eef0e5...

Powered by Google App Engine
This is Rietveld 0eb02b776