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

Issue 1935203002: [Courgette] Using LabelManager to reduce Courgette-apply peak RAM by 25%. (Closed)

Created:
4 years, 7 months ago by huangs
Modified:
4 years, 7 months ago
Reviewers:
Will Harris
CC:
chromium-reviews, grt (UTC plus 2), chrisha, etiennep
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Courgette] Using LabelManager to reduce Courgette-apply peak RAM by 25%. AssemblyProgram previously allocates new Label instances as it parses an executable and emits instructions. This CL replaces the flow by using LabelManager to precompute Labels in one array. This allows us to reduce Courgette-apply peak RAM by 25%, measured by "choke RAM until failure" method. Details: - We precompute Labels in AssemblyProgram::PrecomputeLabels(), which relies on RvaVisitor inherited classes for architecture-specific extraction of abs32 and rel32 targets. - TrimLabel()'s complex post-processing flow is simplified using PrecomputeLabels(), which runs before main file parse. - This requires RemoveUnusedRel32Locations() to update rel32. - Deprecating C_TRIM_FAILED error message. - Moving more common functionality to Disassembler, but duplicating some code for win32-x86 and win32-x64 to follow existing pattern. BUG=613216 Committed: https://crrev.com/c803763b7f4ee725717839b679cadeb1637b6be5 Cr-Commit-Position: refs/heads/master@{#394815}

Patch Set 1 #

Total comments: 8

Patch Set 2 : Cleanup; fix comments. #

Patch Set 3 : Sync; rewrite DisassemblerElf32::RemoveUnusedRel32Locations() for new std::unique_ptr<> usage. #

Patch Set 4 : Sync. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+416 lines, -253 lines) Patch
M courgette/adjustment_method_unittest.cc View 4 chunks +16 lines, -4 lines 0 comments Download
M courgette/assembly_program.h View 3 chunks +16 lines, -14 lines 0 comments Download
M courgette/assembly_program.cc View 6 chunks +36 lines, -86 lines 0 comments Download
M courgette/courgette.h View 1 1 chunk +0 lines, -1 line 0 comments Download
M courgette/disassembler.h View 4 chunks +48 lines, -0 lines 0 comments Download
M courgette/disassembler.cc View 2 chunks +31 lines, -0 lines 0 comments Download
M courgette/disassembler_elf_32.h View 1 2 6 chunks +30 lines, -8 lines 0 comments Download
M courgette/disassembler_elf_32.cc View 1 2 8 chunks +73 lines, -25 lines 0 comments Download
M courgette/disassembler_elf_32_arm.h View 1 chunk +1 line, -1 line 0 comments Download
M courgette/disassembler_elf_32_arm.cc View 1 2 1 chunk +2 lines, -5 lines 0 comments Download
M courgette/disassembler_elf_32_x86.h View 1 chunk +1 line, -1 line 0 comments Download
M courgette/disassembler_elf_32_x86.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M courgette/disassembler_win32_x64.h View 1 chunk +5 lines, -0 lines 0 comments Download
M courgette/disassembler_win32_x64.cc View 1 5 chunks +32 lines, -3 lines 0 comments Download
M courgette/disassembler_win32_x86.h View 1 chunk +5 lines, -0 lines 0 comments Download
M courgette/disassembler_win32_x86.cc View 1 4 chunks +30 lines, -2 lines 0 comments Download
M courgette/encoded_program.h View 2 chunks +12 lines, -3 lines 0 comments Download
M courgette/encoded_program.cc View 2 chunks +45 lines, -31 lines 0 comments Download
M courgette/encoded_program_unittest.cc View 3 chunks +23 lines, -46 lines 0 comments Download
M courgette/image_utils.h View 3 chunks +6 lines, -2 lines 0 comments Download
M courgette/label_manager.h View 1 chunk +0 lines, -4 lines 0 comments Download
M courgette/label_manager.cc View 1 chunk +0 lines, -11 lines 0 comments Download
M courgette/program_detector.cc View 1 chunk +0 lines, -3 lines 0 comments Download
M courgette/rel32_finder_win32_x86.cc View 1 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 22 (8 generated)
huangs
The final CL of the saga (hopefully)! PTAL.
4 years, 7 months ago (2016-05-02 19:08:22 UTC) #2
Will Harris
who is reviewing which parts?
4 years, 7 months ago (2016-05-03 21:25:56 UTC) #3
huangs
You're the main reviewer, please review everything! Added chrisha@ since I think he's interested as ...
4 years, 7 months ago (2016-05-03 21:39:12 UTC) #4
Will Harris
On 2016/05/03 21:39:12, huangs wrote: > You're the main reviewer, please review everything! Added chrisha@ ...
4 years, 7 months ago (2016-05-03 21:50:40 UTC) #7
huangs
Oops typo, changing chrishall@ -> chrisha@.
4 years, 7 months ago (2016-05-03 22:39:26 UTC) #8
Will Harris
https://codereview.chromium.org/1935203002/diff/1/courgette/assembly_program.cc File courgette/assembly_program.cc (right): https://codereview.chromium.org/1935203002/diff/1/courgette/assembly_program.cc#newcode183 courgette/assembly_program.cc:183: const int AssemblyProgram::kLabelLowerLimit = 5; is this value of ...
4 years, 7 months ago (2016-05-04 18:02:26 UTC) #9
huangs
Thanks! Updated, PTAL. https://codereview.chromium.org/1935203002/diff/1/courgette/assembly_program.cc File courgette/assembly_program.cc (right): https://codereview.chromium.org/1935203002/diff/1/courgette/assembly_program.cc#newcode183 courgette/assembly_program.cc:183: const int AssemblyProgram::kLabelLowerLimit = 5; Probably ...
4 years, 7 months ago (2016-05-04 19:34:00 UTC) #10
Will Harris
lgtm
4 years, 7 months ago (2016-05-04 20:35:30 UTC) #11
huangs
Thanks! After talking to chrisha@ and siggi@, I realized that I've been neglecting measuring the ...
4 years, 7 months ago (2016-05-05 15:58:29 UTC) #12
huangs
Experimented *locally* by modifying courgette_tools.cc: #include "base/process/process_metrics.h" ... int main(int argc, const char* argv[]) { ...
4 years, 7 months ago (2016-05-19 17:13:38 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1935203002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1935203002/60001
4 years, 7 months ago (2016-05-19 17:15:06 UTC) #16
Will Harris
still lgtm, looking forward to the results, can you add a BUG= to this CL ...
4 years, 7 months ago (2016-05-19 17:15:27 UTC) #17
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 7 months ago (2016-05-19 18:17:07 UTC) #20
commit-bot: I haz the power
4 years, 7 months ago (2016-05-19 18:18:26 UTC) #22
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/c803763b7f4ee725717839b679cadeb1637b6be5
Cr-Commit-Position: refs/heads/master@{#394815}

Powered by Google App Engine
This is Rietveld 408576698