Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2039233005:
Consider reloc info mode when merging constant pool entries
|
Created:
4 years, 6 months ago by Mircea Trofin Modified:
4 years, 6 months ago CC:
v8-reviews_googlegroups.com, v8-mips-ports_googlegroups.com, v8-ppc-ports_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionThis change allows us to have constants that are equal in
value but with different relocation modes. For instance, the
index of a wasm function and the index of a wasm import -
both may be "2", but they mean different things and are
relocated differently.
Introduced a NONEINTPTR reloc mode. Akin NONE32 and NONE64, this
signifies a "no relocation sized intptr".
Opportunistically: use standard datastructures for tracking
constant pool entries. Compile time is unaffected as shown
by both Compile and Wasm benchmarks, while code
maintainability is improved.
Test to follow in subsequent change.
BUG=v8:5072
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Use standard datastructures for tracking constant pool entries. BUG= ========== to ========== This change allows us to have constants that are equal in value but with different relocation modes. For instance, the index of a wasm function and the index of a wasm import - both may be "2", but they mean different things and are relocated differently. Opportunistically: use standard datastructures for tracking constant pool entries. Compile time is unaffected as shown by both Compile and Wasm benchmarks, while code maintainability is improved. Test to follow in subsequent change. BUG= ==========
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org
https://codereview.chromium.org/2039233005/diff/40001/src/assembler.cc File src/assembler.cc (right): https://codereview.chromium.org/2039233005/diff/40001/src/assembler.cc#newcod... src/assembler.cc:1937: if ((entry_size == kPointerSize) ? entry.IntValueMayBeMergedWith(*it) It seems strange to use carnal knowledge of entry to decide which method to call? Also, just because entry_size != kPointerSize, doesn't seem that means you can merge mismatched relocation types. Or am I missing something? https://codereview.chromium.org/2039233005/diff/40001/src/assembler.h File src/assembler.h (right): https://codereview.chromium.org/2039233005/diff/40001/src/assembler.h#newcode429 src/assembler.h:429: NONEINTPTR, // never recorded intptr value Maybe highlight in the commit message that you're distinguishing this as an rmode ? https://codereview.chromium.org/2039233005/diff/40001/src/assembler.h#newcode... src/assembler.h:1267: return rmode_ == other.rmode_ && value() == other.value(); Why not have this handle checking both the 32/64 case so the callsite doesn't have to do it, which seems odd anyhow?
Description was changed from ========== This change allows us to have constants that are equal in value but with different relocation modes. For instance, the index of a wasm function and the index of a wasm import - both may be "2", but they mean different things and are relocated differently. Opportunistically: use standard datastructures for tracking constant pool entries. Compile time is unaffected as shown by both Compile and Wasm benchmarks, while code maintainability is improved. Test to follow in subsequent change. BUG= ========== to ========== This change allows us to have constants that are equal in value but with different relocation modes. For instance, the index of a wasm function and the index of a wasm import - both may be "2", but they mean different things and are relocated differently. Introduced a NONEINTPTR reloc mode. Akin NONE32 and NONE64, this signifies a "no relocation sized intptr". Opportunistically: use standard datastructures for tracking constant pool entries. Compile time is unaffected as shown by both Compile and Wasm benchmarks, while code maintainability is improved. Test to follow in subsequent change. BUG= ==========
https://codereview.chromium.org/2039233005/diff/40001/src/assembler.cc File src/assembler.cc (right): https://codereview.chromium.org/2039233005/diff/40001/src/assembler.cc#newcod... src/assembler.cc:1937: if ((entry_size == kPointerSize) ? entry.IntValueMayBeMergedWith(*it) On 2016/06/08 00:58:32, bradnelson wrote: > It seems strange to use carnal knowledge of entry to decide which method to > call? Also, just because entry_size != kPointerSize, doesn't seem that means you > can merge mismatched relocation types. Or am I missing something? This is inherited (previous) code, and I agree, overall the logic here isn't ideal. My goal was to initially just nudge the constant pool logic to support categories of constants, and then try to clean this up, but it may be a small enough additional change to do it now. Let me give it a go. https://codereview.chromium.org/2039233005/diff/40001/src/assembler.h File src/assembler.h (right): https://codereview.chromium.org/2039233005/diff/40001/src/assembler.h#newcode429 src/assembler.h:429: NONEINTPTR, // never recorded intptr value On 2016/06/08 00:58:32, bradnelson wrote: > Maybe highlight in the commit message that you're distinguishing this as an > rmode ? Done. https://codereview.chromium.org/2039233005/diff/40001/src/assembler.h#newcode... src/assembler.h:1267: return rmode_ == other.rmode_ && value() == other.value(); On 2016/06/08 00:58:32, bradnelson wrote: > Why not have this handle checking both the 32/64 case so the callsite doesn't > have to do it, which seems odd anyhow? See my previous comment about the scope of this change.
PTAL - refactored more of the original code.
Description was changed from ========== This change allows us to have constants that are equal in value but with different relocation modes. For instance, the index of a wasm function and the index of a wasm import - both may be "2", but they mean different things and are relocated differently. Introduced a NONEINTPTR reloc mode. Akin NONE32 and NONE64, this signifies a "no relocation sized intptr". Opportunistically: use standard datastructures for tracking constant pool entries. Compile time is unaffected as shown by both Compile and Wasm benchmarks, while code maintainability is improved. Test to follow in subsequent change. BUG= ========== to ========== This change allows us to have constants that are equal in value but with different relocation modes. For instance, the index of a wasm function and the index of a wasm import - both may be "2", but they mean different things and are relocated differently. Introduced a NONEINTPTR reloc mode. Akin NONE32 and NONE64, this signifies a "no relocation sized intptr". Opportunistically: use standard datastructures for tracking constant pool entries. Compile time is unaffected as shown by both Compile and Wasm benchmarks, while code maintainability is improved. Test to follow in subsequent change. BUG=v8:5072 ==========
bradnelson@google.com changed reviewers: + bradnelson@google.com
The CQ bit was checked by bradnelson@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2039233005/60001
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/16704)
mtrofin@chromium.org changed reviewers: + rmcilroy@chromium.org
+rmcilroy for the arm files.
Could you clarify a bit on why you need different reloc types to not be shared with each other? Could you just move the WASM reloc types you want to be non-merged into the non-sharable RelocInfo types (above CELL)? My concern with this change is that it means we also can't share identical constants of other RelocInfo types which don't care about the fact they are different (e.g., a POSITION and a STATEMENT_POSITION) which might end up impacting constant pool size. Alternatively, if you still need to share entries within one of these relocinfo types (and so need the reloc type in the ConstantPoolEntry), could you also move the sharing_allowed checks into the ConstantPoolEntry rather than passing that in as a separate argument. Finally, I would really prefer this as two CLs - one which changes the way we merge reloc info, and the other which uses std:vector instead of an array - that way it would be obvious if there is a perf regression on the bots which change is the cause.
On 2016/06/08 12:38:16, rmcilroy wrote: > Could you clarify a bit on why you need different reloc types to not be shared > with each other? > > Could you just move the WASM reloc types you want to be non-merged into the > non-sharable RelocInfo types (above CELL)? The upcoming reloc types - WASM_IMPORT_CALL, WASM_DIRECT_CALL - are as frequent as CODE_TARGETs, so would benefit from merging. > My concern with this change is that > it means we also can't share identical constants of other RelocInfo types which > don't care about the fact they are different (e.g., a POSITION and a > STATEMENT_POSITION) which might end up impacting constant pool size. We can add to the logic in MayBeMerged to special case these modes. Are there more than POSITION and STATEMENT_POSITION that can be cross-merged? I assume those may be just merged with eachother, not, say, with CODE_TARGETs, correct? > Alternatively, if you still need to share entries within one of these relocinfo > types (and so need the reloc type in the ConstantPoolEntry), could you also move > the sharing_allowed checks into the ConstantPoolEntry rather than passing that > in as a separate argument. Oh, good point, will do. > > Finally, I would really prefer this as two CLs - one which changes the way we > merge reloc info, and the other which uses std:vector instead of an array - that > way it would be obvious if there is a perf regression on the bots which change > is the cause. Makes sense. Will split them, and make the necessary adjustments discussed here. FWIW, since I was also concerned about the perf impact, note the compile benchmarks run in Patch Set#1, which is just the std::vector change.
On 2016/06/08 14:41:07, Mircea Trofin wrote: > On 2016/06/08 12:38:16, rmcilroy wrote: > > Could you clarify a bit on why you need different reloc types to not be shared > > with each other? > > > > Could you just move the WASM reloc types you want to be non-merged into the > > non-sharable RelocInfo types (above CELL)? > > The upcoming reloc types - WASM_IMPORT_CALL, WASM_DIRECT_CALL - are as frequent > as > CODE_TARGETs, so would benefit from merging. Just to point out, CODE_TARGETs don't get merged at all, since they might end up getting back-patched to end up pointing at different locations than they start pointing at. Just to check, have you identified situations where merging WASM_IMPORT_CALL / WASM_DIRECT_CALL substantially reduces the constant pool size (i.e., are there runs of code which refer to the same WASM_DIRECT_CALL location and thus benefit from merging)? > > My concern with this change is that > > it means we also can't share identical constants of other RelocInfo types > which > > don't care about the fact they are different (e.g., a POSITION and a > > STATEMENT_POSITION) which might end up impacting constant pool size. > > We can add to the logic in MayBeMerged to special case these modes. Are there > more > than POSITION and STATEMENT_POSITION that can be cross-merged? I assume those > may > be just merged with eachother, not, say, with CODE_TARGETs, correct? We currently cross-merge all entries which would never be GCed or back-patched (everything below CELL), regardless of their reloc-type (so all those which have sharing_allowed). This isn't a problem since they can happily share the same constant pool entry even if they have different relocinfo entries since they won't ever change. I'm guessing WASM_IMPORT_CALL, WASM_DIRECT_CALL might get moved by the GC so we can't do this? > > Alternatively, if you still need to share entries within one of these > relocinfo > > types (and so need the reloc type in the ConstantPoolEntry), could you also > move > > the sharing_allowed checks into the ConstantPoolEntry rather than passing that > > in as a separate argument. > Oh, good point, will do. > > > > Finally, I would really prefer this as two CLs - one which changes the way we > > merge reloc info, and the other which uses std:vector instead of an array - > that > > way it would be obvious if there is a perf regression on the bots which change > > is the cause. > > Makes sense. Will split them, and make the necessary adjustments discussed here. > > FWIW, since I was also concerned about the perf impact, note the compile > benchmarks > run in Patch Set#1, which is just the std::vector change.
On 2016/06/08 15:11:10, rmcilroy wrote: > On 2016/06/08 14:41:07, Mircea Trofin wrote: > > On 2016/06/08 12:38:16, rmcilroy wrote: > > > Could you clarify a bit on why you need different reloc types to not be > shared > > > with each other? > > > > > > Could you just move the WASM reloc types you want to be non-merged into the > > > non-sharable RelocInfo types (above CELL)? > > > > The upcoming reloc types - WASM_IMPORT_CALL, WASM_DIRECT_CALL - are as > frequent > > as > > CODE_TARGETs, so would benefit from merging. > > Just to point out, CODE_TARGETs don't get merged at all, since they might end up > getting back-patched to end up pointing at different locations than they start > pointing at. Just to check, have you identified situations where merging > WASM_IMPORT_CALL / WASM_DIRECT_CALL substantially reduces the constant pool size > (i.e., are there runs of code which refer to the same WASM_DIRECT_CALL location > and thus benefit from merging)? I didn't realize that, about CODE_TARGETs. Re. the other 2, I noticed cases where merging them caused functional defects. In our test corpus, that happened just in a few larger benchmarks. That gives some indication of frequency of occurrence, albeit a bit skewed, since most tests we have aren't quite large bodies of code either. Since I was running on the assumption CODE_TARGETs are merged, I didn't explore not merging the upcoming relocs. I could do that instead. An argument for moving forward with the CL, though, is that it encapsulates the criteria for merging, making this sort of exploration easier to conduct. > > > My concern with this change is that > > > it means we also can't share identical constants of other RelocInfo types > > which > > > don't care about the fact they are different (e.g., a POSITION and a > > > STATEMENT_POSITION) which might end up impacting constant pool size. > > > > We can add to the logic in MayBeMerged to special case these modes. Are there > > more > > than POSITION and STATEMENT_POSITION that can be cross-merged? I assume those > > may > > be just merged with eachother, not, say, with CODE_TARGETs, correct? > > We currently cross-merge all entries which would never be GCed or back-patched > (everything below CELL), regardless of their reloc-type (so all those which have > sharing_allowed). This isn't a problem since they can happily share the same > constant pool entry even if they have different relocinfo entries since they > won't ever change. I'm guessing WASM_IMPORT_CALL, WASM_DIRECT_CALL might get > moved by the GC so we can't do this? > The WASM entries get patched to become actual call targets. Initially, they only contain indices in one of 2 tables: WASM_IMPORT_CALL indexes into the wasm import table, and the other one in the wasm function table. So if a function calls both function "2" and import "2", these will be completely different call targets, once we're done patching the code. > > > > Alternatively, if you still need to share entries within one of these > > relocinfo > > > types (and so need the reloc type in the ConstantPoolEntry), could you also > > move > > > the sharing_allowed checks into the ConstantPoolEntry rather than passing > that > > > in as a separate argument. > > Oh, good point, will do. > > > > > > Finally, I would really prefer this as two CLs - one which changes the way > we > > > merge reloc info, and the other which uses std:vector instead of an array - > > that > > > way it would be obvious if there is a perf regression on the bots which > change > > > is the cause. > > > > Makes sense. Will split them, and make the necessary adjustments discussed > here. > > > > FWIW, since I was also concerned about the perf impact, note the compile > > benchmarks > > run in Patch Set#1, which is just the std::vector change.
Patchset #5 (id:80001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
