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

Issue 699633003: CodeGen: rewrite implementation [3/3] (Closed)

Created:
6 years, 1 month ago by mdempsky
Modified:
6 years, 1 month ago
CC:
chromium-reviews, jln+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@codegen-api-2
Project:
chromium
Visibility:
Public.

Description

CodeGen: rewrite implementation [3/3] This is a complete rewrite of CodeGen from scratch, taking advantage of the fact that a while back we changed PolicyCompiler to not require modifying instructions after they're created. In particular, we no longer need topological sorting because the MakeInstruction call order implicitly sorts instructions for us. Additionally, we no longer need to compute basic blocks even because they were only needed for sorting. It turns out that a simple greedy algorithm with basic memoization suffices for flattening the instruction graph, and we only really need to worry about inserting "jump" instructions when the targets are too far away. With just this, we're able to emit programs that are just as efficient as the old CodeGen implementation did. As a bonus, the new code generator is now fully deterministic because it doesn't create any sets or maps using pointer values as keys. BUG=414363 Committed: https://crrev.com/1fe9104fec2241de3cb547f0580ebb24edc13613 Cr-Commit-Position: refs/heads/master@{#304977}

Patch Set 1 #

Patch Set 2 : Refresh #

Patch Set 3 : Undo spurious comment change #

Patch Set 4 : Sync and add extra code folding unit test #

Patch Set 5 : Might as well use ProgramTest here too #

Patch Set 6 : Use size_t instead of uint32_t as semantically appropriate #

Total comments: 10

Patch Set 7 : Respond to rickyz feedback #

Total comments: 17

Patch Set 8 : Moar comments #

Total comments: 12

Patch Set 9 : Respond to jln feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+195 lines, -765 lines) Patch
M sandbox/linux/BUILD.gn View 1 2 3 4 5 6 1 chunk +0 lines, -3 lines 0 comments Download
M sandbox/linux/sandbox_linux.gypi View 1 2 3 4 5 6 1 chunk +0 lines, -3 lines 0 comments Download
D sandbox/linux/seccomp-bpf/basicblock.h View 1 chunk +0 lines, -49 lines 0 comments Download
D sandbox/linux/seccomp-bpf/basicblock.cc View 1 chunk +0 lines, -13 lines 0 comments Download
M sandbox/linux/seccomp-bpf/codegen.h View 1 2 3 4 5 6 7 3 chunks +38 lines, -77 lines 0 comments Download
M sandbox/linux/seccomp-bpf/codegen.cc View 1 2 3 4 5 6 7 8 1 chunk +108 lines, -550 lines 0 comments Download
M sandbox/linux/seccomp-bpf/codegen_unittest.cc View 1 2 3 4 5 6 7 8 3 chunks +49 lines, -10 lines 0 comments Download
D sandbox/linux/seccomp-bpf/instruction.h View 1 chunk +0 lines, -60 lines 0 comments Download

Messages

Total messages: 14 (2 generated)
mdempsky
6 years, 1 month ago (2014-11-04 00:56:25 UTC) #2
rickyz (no longer on Chrome)
lgtm Wow, that's one big cleanup :-) Would be interesting to see what the diff ...
6 years, 1 month ago (2014-11-12 23:43:50 UTC) #3
mdempsky
https://codereview.chromium.org/699633003/diff/100001/sandbox/linux/seccomp-bpf/codegen.cc File sandbox/linux/seccomp-bpf/codegen.cc (right): https://codereview.chromium.org/699633003/diff/100001/sandbox/linux/seccomp-bpf/codegen.cc#newcode23 sandbox/linux/seccomp-bpf/codegen.cc:23: out->assign(program_.rbegin() + Offset(head), program_.rend()); On 2014/11/12 23:43:50, rickyz wrote: ...
6 years, 1 month ago (2014-11-18 01:07:30 UTC) #4
mdempsky
On 2014/11/12 23:43:50, rickyz wrote: > Wow, that's one big cleanup :-) Would be interesting ...
6 years, 1 month ago (2014-11-18 01:17:35 UTC) #5
jln (very slow on Chromium)
This looks like a pretty spectacular cleanup, thanks! I'm pretty sure this is ok as-is, ...
6 years, 1 month ago (2014-11-19 00:32:05 UTC) #6
mdempsky
PTAL! > This looks like a pretty spectacular cleanup, thanks! :-) > In general: I ...
6 years, 1 month ago (2014-11-19 03:05:23 UTC) #7
mdempsky
I plan on investigating the WithinRange optimizations further in a followup CL, but some quick ...
6 years, 1 month ago (2014-11-19 04:11:54 UTC) #8
jln (very slow on Chromium)
lgtm This is really elegant, love it! :) https://chromiumcodereview.appspot.com/699633003/diff/140001/sandbox/linux/seccomp-bpf/codegen.cc File sandbox/linux/seccomp-bpf/codegen.cc (right): https://chromiumcodereview.appspot.com/699633003/diff/140001/sandbox/linux/seccomp-bpf/codegen.cc#newcode67 sandbox/linux/seccomp-bpf/codegen.cc:67: // ...
6 years, 1 month ago (2014-11-20 02:00:03 UTC) #9
mdempsky
Responded to a couple of your points, but unfortunately most of them I don't have ...
6 years, 1 month ago (2014-11-20 02:53:22 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/699633003/160001
6 years, 1 month ago (2014-11-20 02:54:31 UTC) #12
commit-bot: I haz the power
Committed patchset #9 (id:160001)
6 years, 1 month ago (2014-11-20 04:53:03 UTC) #13
commit-bot: I haz the power
6 years, 1 month ago (2014-11-20 04:53:39 UTC) #14
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/1fe9104fec2241de3cb547f0580ebb24edc13613
Cr-Commit-Position: refs/heads/master@{#304977}

Powered by Google App Engine
This is Rietveld 408576698