[Courgette] Reduce AssemblyProgram to reduce Courgette-apply RAM floor and disk churn.

[Courgette] Reduce AssemblyProgram to reduce Courgette-apply RAM floor and disk churn.

This is the main CL of the AssemblyProgram reduction effort, which makes
Disassembler emit instructions directly to EncodedProgram, and bypass
AssemblyProgram, which becomes a holder of LabelManager and Label
Annotation. Another big change is that Disassembler usage is exposed to
high-level flows (rather than having it hidden inside
ParseDetectedExecutable*()). Details:

- Remove ParseDetectedExecutable*() and expose DetectDisassembler().
- AssemblyProgram: Remove:
  - Entire Instruction class hierarchy.
  - Allocated instruction in the heap (including preallocated DEFBYTE
    instances).
  - enum OP {}, which duplicates EncodedProgram::OP.
  - InstructionVector of pointers, which takes 100's of MB on disk.
  - Encode() interface.
- AssemblyProgram: Add:
  - AnnotateLabels() to generate Label Annotation directly from a
    generator (injected by Disassembler), using LabelReceptor template
    class for counting and storage.
  - PrepareEncodedProgram() to inject data needed (e.g., Label) to
    EncodedProgram.
- Disassembler: Disassemble() is split into CreateProgram() and
  DisassembleAndEncode().
- EncodedProgram: Old AssemblyProgram::Encode() code now moved to
  InstructionStoreReceptor. New GenerateInstruction() is added to
  receive generator from Disassemblyprogram.
- CourgetteFlow: Disassembler explicitly managed:
  - Remove ReadAssemblyProgramFromBuffer().
  - Add ReadDisassemblerFromBuffer().
  - Add CreateEncodedProgramFromDisassemblerAndAssemblyProgram().
- High-level flows in courgette_tool.cc, patch_generator_x86_32.h,
  patcher_x86_32.h: Using new more complex flow that involves
  Disassembler; using ASCII graphics in comment to illustrate.
- Unit tests updates.

Documentation update will be done in a follow-up.

BUG=660980
Review-Url: https://codereview.chromium.org/2854113002
Cr-Commit-Position: refs/heads/master@{#472907}
Committed: https://chromium.googlesource.com/chromium/src/+/88451332849bb3632182e4299f4c2b64a240b6c8

[huangs, 2017-05-03 22:43:42]

Description was changed from

==========
[Courgette] Reduce AssemblyProgram to reduce Courgette-apply RAM floor and disk
churn.

This is the main CL of the AssemblyProgram reduction effort.

BUG=660980
==========

to

==========
[Courgette] Reduce AssemblyProgram to reduce Courgette-apply RAM floor and disk
churn.

This is the main CL of the AssemblyProgram reduction effort, which makes
Disassembler emit instructions directly to EncodedProgram, and bypass
AssemblyProgram, which becomes a holder of LabelManager and Label
Annotation. Another big change is that Disassembler usage is exposed to
high-level flows (rather than having it hidden inside
ParseDetectedExecutable*()). Details:

- Remove ParseDetectedExecutable*() and expose DetectDisassembler().
- AssemblyProgram: Remove:
  - Entire Instruction class hierarchy.
  - Allocated instruction in the heap (including preallocated DEFBYTE
    instances).
  - enum OP {}, which duplicates EncodedProgram::OP.
  - InstructionVector of pointers, which takes 100's of MB on disk.
  - Encode() interface.
- AssemblyProgram: Add:
  - AnnotateLabels() to generate Label Annotation directly from a
    generator (injected by Disassembler), using LabelReceptor template
    class for counting and storage.
  - PrepareEncodedProgram() to inject data needed (e.g., Label) to
    EncodedProgram.
- Disassembler: Disassemble() is split into CreateProgram() and
  DisassembleAndEncode().
- EncodedProgram: Old AssemblyProgram::Encode() code now moved to
  InstructionStoreReceptor. New GenerateInstruction() is added to
  receive generator from Disassemblyprogram.
- CourgetteFlow: Disassembler explicitly managed:
  - Remove ReadAssemblyProgramFromBuffer().
  - Add ReadDisassemblerFromBuffer().
  - Add CreateEncodedProgramFromDisassemblerAndAssemblyProgram().
- High-level flows in courgette_tool.cc, patch_generator_x86_32.h,
  patcher_x86_32.h: Using new more complex flow that involves
  Disassembler; using ASCII graphics in comment to illustrate.
- Unit tests updates.

Documentation update will be done in a follow-up.

BUG=660980
==========

[huangs, 2017-05-03 22:43:42]

huangs@chromium.org changed reviewers:
+ wfh@chromium.org

[huangs, 2017-05-03 22:45:50]

PTAL. But before committing this, I'm thinking of making a CL to better track
peak memory in MemoryAllocator and print it via verbose output, so I have more
precise values on the "saving 200 - 400 MB disk churn" claim that were obtained
from calculation. Thanks!

[Will Harris, 2017-05-05 21:36:51]

I tried and failed to find something concrete to even nit on here. Very
impressive CL. lgtm % a comment.

I agree it would be nice to have some metrics here to determine the
improvements. Can this be done on the perf bots maybe?

[Will Harris, 2017-05-05 22:26:22]

forgot to publish, sorry

https://codereview.chromium.org/2854113002/diff/20001/courgette/courgette_too...
File courgette/courgette_tool.cc (right):

https://codereview.chromium.org/2854113002/diff/20001/courgette/courgette_too...
courgette/courgette_tool.cc:188: flow.DestroyDisassembler(flow.OLD);
can these create and destroys be managed with smart pointer lifetimes instead of
explicit calls?

[huangs, 2017-05-14 04:27:56]

Thanks!  To measure "disk churn" I made experiment CL
http://crrev.com/2883773002/ that tracks TempMapping usage (vs. merged version
with this CL) and ran Courgette-apply for 54-55 for 32-bit and 64-bit. Results:

*** Before: 32-bit ***
Sum: 2,010,686,965
Max: 469,762,069
*** After: 32-bit ***
Sum: 1,238,934,965
Max: 430,624,436

*** Before: 64-bit ***
Sum: 2,489,706,546
Max: 494,006,621
*** After: 64-bit ***
Sum: 1,483,073,522
Max: 494,006,621

Total churn decreased by 770 MB for 32-bit, and 1 GB for 64-bit; which is
expected since this accumulates churn from each file inside chrome.7z.

Peak churn decreased only by 40 MB for 32-bit, and none by 64-bit. I think this
is because peak occurs at the final ensemble patching step, which drowns out
peak at AssemblyProgram.

So, in addition to RAM floor reduction, the CL reduces total disk churn when
memory is constrained.  Moreover, when there is enough RAM such that temp files
can exist in memory, Courgette would still have 400 - 500 MB of extra RAM.

Planning to commit on Monday afternoon.

https://codereview.chromium.org/2854113002/diff/20001/courgette/courgette_too...
File courgette/courgette_tool.cc (right):

https://codereview.chromium.org/2854113002/diff/20001/courgette/courgette_too...
courgette/courgette_tool.cc:188: flow.DestroyDisassembler(flow.OLD);
Yes, then we won't need the flow.Destroy*() functions. To implement this we can
add reference counting inside CourgetteFlow::Data, update them on creation & use
(with error handing), and deallocate on the 1->0 transition. I judge this to be
over-complication though, but can do it on a follow-up if you insist?

[huangs, 2017-05-15 21:25:37]

The CQ bit was checked by huangs@chromium.org

[huangs, 2017-05-15 21:25:38]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/2854113002/#ps40001
(title: "Sync.")

[commit-bot: I haz the power, 2017-05-15 21:26:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 22:13:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 22:13:14]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[huangs, 2017-05-16 05:47:02]

The CQ bit was checked by huangs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 05:47:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 06:55:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 06:55:09]

Dry run: This issue passed the CQ dry run.

[huangs, 2017-05-16 17:32:16]

huangs@chromium.org changed reviewers:
+ ochang@chromium.org

[huangs, 2017-05-16 17:32:16]

OWNER review to ochang@ for:

 testing/libfuzzer/fuzzers/courgette_fuzzer.cc

PTAL. Thanks!

[huangs, 2017-05-17 19:46:29]

Ping ochang@ for review.

[Oliver Chang (dont use), 2017-05-17 19:54:28]

ochang@google.com changed reviewers:
+ ochang@google.com

[Oliver Chang (dont use), 2017-05-17 19:54:28]

lgtm

[huangs, 2017-05-17 20:14:53]

Thanks. Committing!

[huangs, 2017-05-17 20:15:09]

The CQ bit was checked by huangs@chromium.org

[huangs, 2017-05-17 20:15:09]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/2854113002/#ps60001
(title: "Update courgette_fuzzer in libfuzzer.")

[commit-bot: I haz the power, 2017-05-17 20:16:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 20:58:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 20:58:23]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Will Harris, 2017-05-17 21:14:18]

ochang replied on the wrong account.

[Oliver Chang (dont use), 2017-05-18 03:50:19]

On 2017/05/17 21:14:18, Will Harris wrote:
> ochang replied on the wrong account.

woops, lgtm

[Oliver Chang, 2017-05-18 03:50:56]

On 2017/05/18 03:50:19, Oliver Chang (dont use) wrote:
> On 2017/05/17 21:14:18, Will Harris wrote:
> > ochang replied on the wrong account.
> 
> woops, lgtm

trying again... lgtm

[huangs, 2017-05-18 14:58:20]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2017-05-18 14:59:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 18:15:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 18:15:39]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[huangs, 2017-05-18 18:16:36]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2017-05-18 18:18:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 19:50:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495131396812830,
"parent_rev": "cb5adebb5e8db8146d7ad2fb5a8c6219561ae28c", "commit_rev":
"88451332849bb3632182e4299f4c2b64a240b6c8"}

[commit-bot: I haz the power, 2017-05-18 19:50:48]

Description was changed from

==========
[Courgette] Reduce AssemblyProgram to reduce Courgette-apply RAM floor and disk
churn.

This is the main CL of the AssemblyProgram reduction effort, which makes
Disassembler emit instructions directly to EncodedProgram, and bypass
AssemblyProgram, which becomes a holder of LabelManager and Label
Annotation. Another big change is that Disassembler usage is exposed to
high-level flows (rather than having it hidden inside
ParseDetectedExecutable*()). Details:

- Remove ParseDetectedExecutable*() and expose DetectDisassembler().
- AssemblyProgram: Remove:
  - Entire Instruction class hierarchy.
  - Allocated instruction in the heap (including preallocated DEFBYTE
    instances).
  - enum OP {}, which duplicates EncodedProgram::OP.
  - InstructionVector of pointers, which takes 100's of MB on disk.
  - Encode() interface.
- AssemblyProgram: Add:
  - AnnotateLabels() to generate Label Annotation directly from a
    generator (injected by Disassembler), using LabelReceptor template
    class for counting and storage.
  - PrepareEncodedProgram() to inject data needed (e.g., Label) to
    EncodedProgram.
- Disassembler: Disassemble() is split into CreateProgram() and
  DisassembleAndEncode().
- EncodedProgram: Old AssemblyProgram::Encode() code now moved to
  InstructionStoreReceptor. New GenerateInstruction() is added to
  receive generator from Disassemblyprogram.
- CourgetteFlow: Disassembler explicitly managed:
  - Remove ReadAssemblyProgramFromBuffer().
  - Add ReadDisassemblerFromBuffer().
  - Add CreateEncodedProgramFromDisassemblerAndAssemblyProgram().
- High-level flows in courgette_tool.cc, patch_generator_x86_32.h,
  patcher_x86_32.h: Using new more complex flow that involves
  Disassembler; using ASCII graphics in comment to illustrate.
- Unit tests updates.

Documentation update will be done in a follow-up.

BUG=660980
==========

to

==========
[Courgette] Reduce AssemblyProgram to reduce Courgette-apply RAM floor and disk
churn.

This is the main CL of the AssemblyProgram reduction effort, which makes
Disassembler emit instructions directly to EncodedProgram, and bypass
AssemblyProgram, which becomes a holder of LabelManager and Label
Annotation. Another big change is that Disassembler usage is exposed to
high-level flows (rather than having it hidden inside
ParseDetectedExecutable*()). Details:

- Remove ParseDetectedExecutable*() and expose DetectDisassembler().
- AssemblyProgram: Remove:
  - Entire Instruction class hierarchy.
  - Allocated instruction in the heap (including preallocated DEFBYTE
    instances).
  - enum OP {}, which duplicates EncodedProgram::OP.
  - InstructionVector of pointers, which takes 100's of MB on disk.
  - Encode() interface.
- AssemblyProgram: Add:
  - AnnotateLabels() to generate Label Annotation directly from a
    generator (injected by Disassembler), using LabelReceptor template
    class for counting and storage.
  - PrepareEncodedProgram() to inject data needed (e.g., Label) to
    EncodedProgram.
- Disassembler: Disassemble() is split into CreateProgram() and
  DisassembleAndEncode().
- EncodedProgram: Old AssemblyProgram::Encode() code now moved to
  InstructionStoreReceptor. New GenerateInstruction() is added to
  receive generator from Disassemblyprogram.
- CourgetteFlow: Disassembler explicitly managed:
  - Remove ReadAssemblyProgramFromBuffer().
  - Add ReadDisassemblerFromBuffer().
  - Add CreateEncodedProgramFromDisassemblerAndAssemblyProgram().
- High-level flows in courgette_tool.cc, patch_generator_x86_32.h,
  patcher_x86_32.h: Using new more complex flow that involves
  Disassembler; using ASCII graphics in comment to illustrate.
- Unit tests updates.

Documentation update will be done in a follow-up.

BUG=660980

Review-Url: https://codereview.chromium.org/2854113002
Cr-Commit-Position: refs/heads/master@{#472907}
Committed:
https://chromium.googlesource.com/chromium/src/+/88451332849bb3632182e4299f4c...
==========

[commit-bot: I haz the power, 2017-05-18 19:50:50]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/88451332849bb3632182e4299f4c...

[Will Harris, 2017-05-18 21:46:50]

🎆