Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2081703002:
Use zig-zag encoding in the source position table. (Closed)
DescriptionUse zig-zag encoding in the source position table.
R=vogelheim@chromium.org
Committed: https://crrev.com/679372852865ced10e4d8ccd756596c8d76fc015
Cr-Commit-Position: refs/heads/master@{#37170}
Patch Set 1 #Patch Set 2 : fix #
Total comments: 3
Patch Set 3 : addressed comments #
Messages
Total messages: 26 (10 generated)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2081703002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/9042)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2081703002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Drive-by: Is this smaller size for typical code than the current approach? I thought we had very few negative numbers in the source position table, if so, isn't zigzag encoding a bit larger on average?
On 2016/06/20 16:13:24, rmcilroy wrote: > Drive-by: Is this smaller size for typical code than the current approach? I > thought we had very few negative numbers in the source position table, if so, > isn't zigzag encoding a bit larger on average? Ah yes. I forgot to add the rationale here. Negative numbers are in fact not so uncommon. (I added a printf for the to-be-encoded value, and piped to sort -n | uniq -c). The result is that, when running sunspider with --ignition, the size of all source position tables account for 12.9kB prior to this patch and 12.2kB after. That on its own is not a lot. But I'm planning to migrate source position info on other code objects from reloc info to source position tables. With the same test, we are looking at a difference between 40kB before and 35kB after the patch.
lgtm https://codereview.chromium.org/2081703002/diff/20001/src/interpreter/source-... File src/interpreter/source-position-table.cc (right): https://codereview.chromium.org/2081703002/diff/20001/src/interpreter/source-... src/interpreter/source-position-table.cc:53: value = ((value << 1) ^ (value >> kShift)); nitpick: This relies on >> doing sign extension on negative values. I think this is implementation defined in C++. (Not sure this is truly an issue; but... well.) https://codereview.chromium.org/2081703002/diff/20001/src/interpreter/source-... src/interpreter/source-position-table.cc:69: // Since bytecode_offset is not negative, we use sign to encode is_statement. super nitpick: double space between "we use".
https://codereview.chromium.org/2081703002/diff/20001/src/interpreter/source-... File src/interpreter/source-position-table.cc (right): https://codereview.chromium.org/2081703002/diff/20001/src/interpreter/source-... src/interpreter/source-position-table.cc:53: value = ((value << 1) ^ (value >> kShift)); On 2016/06/21 14:11:12, vogelheim wrote: > nitpick: This relies on >> doing sign extension on negative values. I think this > is implementation defined in C++. (Not sure this is truly an issue; but... > well.) So... two's complement for negative integer representation is also undefined behavior, which is apparently the cause for sign extension not being defined in the first place. We heavily rely on two's complement here, so... If it turns out that on some platform we cannot rely on this, we can still use branching and have something to the effect of (value < 0) ? -(value*2) + 1 : value*2
The CQ bit was checked by yangguo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2081703002/#ps40001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2081703002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/17675)
yangguo@chromium.org changed reviewers: + rmcilroy@chromium.org
Ross, I still need an owner's review. Please take a look.
Lgtm, thanks for the explanation (weirdly I didn't get any emails until your last reply).
The CQ bit was checked by yangguo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2081703002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use zig-zag encoding in the source position table. R=vogelheim@chromium.org ========== to ========== Use zig-zag encoding in the source position table. R=vogelheim@chromium.org Committed: https://crrev.com/679372852865ced10e4d8ccd756596c8d76fc015 Cr-Commit-Position: refs/heads/master@{#37170} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/679372852865ced10e4d8ccd756596c8d76fc015 Cr-Commit-Position: refs/heads/master@{#37170} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
