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

Issue 2055343002: Courgette: Add static method QuickDetect() to optimize program detection. (Closed)

Created:
4 years, 6 months ago by etiennep
Modified:
4 years, 5 months ago
Reviewers:
huangs, Will Harris
CC:
chromium-reviews, wfh+watch_chromium.org, huangs+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Courgette: Add static method QuickDetect() to optimize program detection. Old way: The detection of executables is achieve by allocating an instance of each Disassembler classes and trying to call ParseHeader() to see if it succeed. This operation is done many times during FindEmbeddedElements() step, which takes ~4% of patch generation time. New way: Using QuickDetect(), which executes only a quick preliminary check, we avoid useless allocation of Disassembler objects unless there's a high probability of successfully detecting a valid executable. This change reduces the execution time of FindEmbeddedElements() by 95%. BUG=619167 Committed: https://crrev.com/5059bca628d156901f9d5c0ef06c0047fa5e5117 Cr-Commit-Position: refs/heads/master@{#404433}

Patch Set 1 #

Total comments: 10

Patch Set 2 : Rename QuickDetects to QuickDetect #

Patch Set 3 : Add unittest #

Patch Set 4 : Replace void* by uint8_t* #

Patch Set 5 : Nit cleanup #

Total comments: 17

Patch Set 6 : Propagate uint8_t* to unittests and Nits #

Total comments: 12

Patch Set 7 : Nit cleanup #

Total comments: 5

Patch Set 8 : Correct comment mistake #

Patch Set 9 : sync #

Unified diffs Side-by-side diffs Delta from patch set Stats (+253 lines, -52 lines) Patch
M courgette/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M courgette/courgette.gyp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M courgette/disassembler.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M courgette/disassembler.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M courgette/disassembler_elf_32.h View 1 2 3 4 5 6 2 chunks +8 lines, -1 line 0 comments Download
M courgette/disassembler_elf_32.cc View 1 2 3 4 5 2 chunks +28 lines, -3 lines 0 comments Download
M courgette/disassembler_elf_32_arm.h View 1 2 3 4 5 6 7 2 chunks +6 lines, -1 line 0 comments Download
M courgette/disassembler_elf_32_arm.cc View 1 2 3 1 chunk +2 lines, -3 lines 0 comments Download
M courgette/disassembler_elf_32_x86.h View 1 2 3 4 5 6 7 2 chunks +6 lines, -1 line 0 comments Download
M courgette/disassembler_elf_32_x86.cc View 1 2 3 1 chunk +2 lines, -3 lines 0 comments Download
M courgette/disassembler_elf_32_x86_unittest.cc View 1 2 3 4 5 6 2 chunks +5 lines, -4 lines 0 comments Download
M courgette/disassembler_win32.h View 1 2 3 4 5 6 7 1 chunk +6 lines, -1 line 0 comments Download
M courgette/disassembler_win32.cc View 1 2 3 4 5 6 2 chunks +31 lines, -1 line 0 comments Download
M courgette/disassembler_win32_x64.h View 1 2 3 4 5 6 7 1 chunk +7 lines, -1 line 0 comments Download
M courgette/disassembler_win32_x64.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M courgette/disassembler_win32_x64_unittest.cc View 1 2 3 4 5 3 chunks +6 lines, -3 lines 0 comments Download
M courgette/disassembler_win32_x86.h View 1 2 3 4 5 6 7 1 chunk +7 lines, -1 line 0 comments Download
M courgette/disassembler_win32_x86.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M courgette/disassembler_win32_x86_unittest.cc View 1 2 3 4 5 3 chunks +6 lines, -3 lines 0 comments Download
M courgette/program_detector.h View 1 2 3 4 5 6 3 chunks +23 lines, -2 lines 0 comments Download
M courgette/program_detector.cc View 1 2 3 2 chunks +23 lines, -19 lines 0 comments Download
A courgette/program_detector_unittest.cc View 1 2 3 4 5 1 chunk +80 lines, -0 lines 0 comments Download

Messages

Total messages: 44 (14 generated)
etiennep
PTAL
4 years, 6 months ago (2016-06-10 18:39:39 UTC) #2
huangs
Looks good! Some slightly involved suggestions, and please add unit tests. https://codereview.chromium.org/2055343002/diff/1/courgette/disassembler_win32.cc File courgette/disassembler_win32.cc (right): ...
4 years, 6 months ago (2016-06-10 21:10:23 UTC) #3
Will Harris
can you clarify which parts you would like huangs to review, and which parts you ...
4 years, 6 months ago (2016-06-10 21:13:17 UTC) #4
huangs
In this case it's to have both us look through everything; we have be careful ...
4 years, 6 months ago (2016-06-10 21:18:48 UTC) #5
huangs
Added tracking bug: http://crbug.com/619167.
4 years, 6 months ago (2016-06-10 21:33:48 UTC) #6
Will Harris
reduces by 95%?! so 0.05x the current time? (3 seconds instead of 1 minute?) or ...
4 years, 6 months ago (2016-06-10 22:41:32 UTC) #8
etiennep
On 2016/06/10 22:41:32, Will Harris wrote: > reduces by 95%?! so 0.05x the current time? ...
4 years, 6 months ago (2016-06-13 15:35:35 UTC) #9
etiennep
PTAnL https://codereview.chromium.org/2055343002/diff/1/courgette/disassembler_win32.cc File courgette/disassembler_win32.cc (right): https://codereview.chromium.org/2055343002/diff/1/courgette/disassembler_win32.cc#newcode656 courgette/disassembler_win32.cc:656: bool DisassemblerWin32::QuickDetects(const uint8_t* start, On 2016/06/10 21:10:22, huangs ...
4 years, 6 months ago (2016-06-13 17:50:22 UTC) #10
huangs
Just NITs. Also, please update courgette.gyp . https://codereview.chromium.org/2055343002/diff/80001/courgette/disassembler_elf_32.cc File courgette/disassembler_elf_32.cc (right): https://codereview.chromium.org/2055343002/diff/80001/courgette/disassembler_elf_32.cc#newcode276 courgette/disassembler_elf_32.cc:276: bool DisassemblerElf32::QuickDetect(const ...
4 years, 6 months ago (2016-06-13 18:30:29 UTC) #11
huangs
https://codereview.chromium.org/2055343002/diff/80001/courgette/program_detector_unittest.cc File courgette/program_detector_unittest.cc (right): https://codereview.chromium.org/2055343002/diff/80001/courgette/program_detector_unittest.cc#newcode50 courgette/program_detector_unittest.cc:50: size_t detected_length; Oops I mean size_t detected_length = 0;
4 years, 6 months ago (2016-06-13 18:34:07 UTC) #12
Will Harris
my pass. but looks generally good. thanks! https://codereview.chromium.org/2055343002/diff/80001/courgette/disassembler.cc File courgette/disassembler.cc (right): https://codereview.chromium.org/2055343002/diff/80001/courgette/disassembler.cc#newcode38 courgette/disassembler.cc:38: start_ = ...
4 years, 6 months ago (2016-06-13 19:15:21 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2055343002/100001
4 years, 6 months ago (2016-06-14 17:24:40 UTC) #16
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 6 months ago (2016-06-14 18:38:21 UTC) #18
etiennep
PTAnL. https://codereview.chromium.org/2055343002/diff/80001/courgette/disassembler.cc File courgette/disassembler.cc (right): https://codereview.chromium.org/2055343002/diff/80001/courgette/disassembler.cc#newcode38 courgette/disassembler.cc:38: start_ = reinterpret_cast<const uint8_t*>(start); On 2016/06/13 19:15:21, Will ...
4 years, 6 months ago (2016-06-14 21:16:38 UTC) #19
huangs
Nice! LGTM with nits. Please also edit the CL description: - Title: Mention Courgette somewhere. ...
4 years, 6 months ago (2016-06-15 00:13:05 UTC) #20
etiennep
Hopefully the final round! https://chromiumcodereview.appspot.com/2055343002/diff/100001/courgette/disassembler_elf_32.h File courgette/disassembler_elf_32.h (right): https://chromiumcodereview.appspot.com/2055343002/diff/100001/courgette/disassembler_elf_32.h#newcode126 courgette/disassembler_elf_32.h:126: // which will be checked ...
4 years, 6 months ago (2016-06-15 20:38:07 UTC) #22
etiennep
Hopefully the final round!
4 years, 6 months ago (2016-06-15 20:38:14 UTC) #23
Will Harris
On 2016/06/15 20:38:14, etiennep wrote: > Hopefully the final round! the first line of your ...
4 years, 6 months ago (2016-06-15 21:48:22 UTC) #24
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2055343002/120001
4 years, 6 months ago (2016-06-16 20:07:16 UTC) #27
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 6 months ago (2016-06-16 21:45:31 UTC) #29
etiennep
On 2016/06/15 21:48:22, Will Harris wrote: > On 2016/06/15 20:38:14, etiennep wrote: > > Hopefully ...
4 years, 6 months ago (2016-06-17 15:13:25 UTC) #31
huangs
Still LGTM, but with a comment NIT. https://codereview.chromium.org/2055343002/diff/120001/courgette/disassembler_elf_32_arm.h File courgette/disassembler_elf_32_arm.h (right): https://codereview.chromium.org/2055343002/diff/120001/courgette/disassembler_elf_32_arm.h#newcode31 courgette/disassembler_elf_32_arm.h:31: // Returns ...
4 years, 6 months ago (2016-06-17 15:21:57 UTC) #32
etiennep
Fixed some more Nit. PTAnL
4 years, 6 months ago (2016-06-17 15:52:56 UTC) #33
etiennep
Ping to Will, if you're back on your feet.
4 years, 5 months ago (2016-07-08 16:49:18 UTC) #34
Will Harris
On 2016/07/08 16:49:18, etiennep wrote: > Ping to Will, if you're back on your feet. ...
4 years, 5 months ago (2016-07-08 16:54:33 UTC) #35
etiennep
On 2016/07/08 16:54:33, Will Harris wrote: > On 2016/07/08 16:49:18, etiennep wrote: > > Ping ...
4 years, 5 months ago (2016-07-08 16:57:49 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/2055343002/160001
4 years, 5 months ago (2016-07-08 16:58:16 UTC) #39
commit-bot: I haz the power
Committed patchset #9 (id:160001)
4 years, 5 months ago (2016-07-08 17:55:34 UTC) #41
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-08 17:56:05 UTC) #42
commit-bot: I haz the power
4 years, 5 months ago (2016-07-08 17:56:48 UTC) #44
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/5059bca628d156901f9d5c0ef06c0047fa5e5117
Cr-Commit-Position: refs/heads/master@{#404433}

Powered by Google App Engine
This is Rietveld 408576698