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

Issue 125510044: Integrated the ZebraBlockHeap to the BlockHeapManager (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by peterssen
Modified:
9 years, 7 months ago
CC:
sawbuck-changes_googlegroups.com
Base URL:
http://sawbuck.googlecode.com/svn/trunk
Visibility:
Public.

Description

Integrated the ZebraBlockHeap to the BlockHeapManager. There are some details missing since the HeapManager is not finished yet. I'm sorry for the delay. I faced not one, but two bugs with non-deterministic behaviour, most times the tests passed, but a very few times just failed on random places. It was difficult to debug since I had no idea where or when it was going to break. Those bugs are fixed now, ironically were caused by use after free. The good part is that now I have a very good idea how everything works since I had to dig deep to discover the bugs. BUG= R=chrisha@chromium.org Committed: https://code.google.com/p/sawbuck/source/detail?r=2272

Patch Set 1 : rebase #

Total comments: 10

Patch Set 2 : New approach, cleaner and simpler #

Total comments: 49

Patch Set 3 : Fixes after review + comments #

Total comments: 7

Patch Set 4 : Fixed nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+352 lines, -54 lines) Patch
M syzygy/agent/asan/heap_managers/block_heap_manager.h View 1 2 3 5 chunks +23 lines, -4 lines 0 comments Download
M syzygy/agent/asan/heap_managers/block_heap_manager.cc View 1 2 3 8 chunks +81 lines, -37 lines 0 comments Download
M syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc View 1 2 3 12 chunks +248 lines, -13 lines 0 comments Download

Messages

Total messages: 11
peterssen
PTAL. Not perfect, since there are a few things missing, but is a good start.
9 years, 7 months ago (2014-08-21 19:11:31 UTC) #1
Sébastien Marchand
A general comment about the design. https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_managers/block_heap_manager.cc File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_managers/block_heap_manager.cc#newcode45 syzygy/agent/asan/heap_managers/block_heap_manager.cc:45: // the memory ...
9 years, 7 months ago (2014-08-21 20:28:18 UTC) #2
peterssen
PTAL. I refactored the integration using the old approach, now the ZebraBlockHeap is treated the ...
9 years, 7 months ago (2014-08-22 19:19:49 UTC) #3
Sébastien Marchand
Looking really good ! I haven't reviewed the unittests in details but the general implementation ...
9 years, 7 months ago (2014-08-22 20:27:46 UTC) #4
chrisha
https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_managers/block_heap_manager.cc File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_managers/block_heap_manager.cc#newcode55 syzygy/agent/asan/heap_managers/block_heap_manager.cc:55: zebra_block_heap_ = new ZebraBlockHeap(1024 * 1024, On 2014/08/22 20:27:46, ...
9 years, 7 months ago (2014-08-25 19:27:48 UTC) #5
Sébastien Marchand
more comments. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_managers/block_heap_manager.cc File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_managers/block_heap_manager.cc#newcode67 syzygy/agent/asan/heap_managers/block_heap_manager.cc:67: DestroyHeapUnlocked(iter_heaps->first, iter_heaps->second); set the zebra heap pointer ...
9 years, 7 months ago (2014-08-25 19:31:30 UTC) #6
peterssen
PTAL. Many fixes, zebra heap initialization delayed until enabled... https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_managers/block_heap_manager.cc File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_managers/block_heap_manager.cc#newcode55 syzygy/agent/asan/heap_managers/block_heap_manager.cc:55: ...
9 years, 7 months ago (2014-08-26 18:50:19 UTC) #7
chrisha
A handful of nits, but otherwise looks good to me! https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_managers/block_heap_manager.cc File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_managers/block_heap_manager.cc#newcode250 ...
9 years, 7 months ago (2014-08-26 19:43:45 UTC) #8
chrisha
So.... lgtm with nits :)
9 years, 7 months ago (2014-08-26 19:43:56 UTC) #9
peterssen
On 2014/08/26 19:43:56, chrisha wrote: > So.... lgtm with nits :) Nits fixed, commiting.
9 years, 7 months ago (2014-08-26 20:30:45 UTC) #10
peterssen
9 years, 7 months ago (2014-08-26 20:31:46 UTC) #11
Message was sent while issue was closed.
Committed patchset #4 manually as 2272 (presubmit successful).
Sign in to reply to this message.

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