Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2044283003:
Use standard datastructures for tracking constant pool entries. (Closed)
|
Created:
4 years, 6 months ago by Mircea Trofin Modified:
4 years, 6 months ago Reviewers:
rmcilroy 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. |
DescriptionUse standard datastructures for tracking constant pool entries.
This improves maintainability. The Compile and Wasm benchmarks,
tracking compile time, show no regression.
BUG=
Committed: https://crrev.com/88a92c47f87690772a5dbede71a103a801a39b94
Cr-Commit-Position: refs/heads/master@{#36834}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Messages
Total messages: 16 (6 generated)
Description was changed from ========== Use standard datastructures for tracking constant pool entries. This improves maintainability. The Compile and Wasm benchmarks, tracking compile time, show no regression. BUG= ========== to ========== Use standard datastructures for tracking constant pool entries. This improves maintainability. The Compile and Wasm benchmarks, tracking compile time, show no regression. BUG= ==========
mtrofin@chromium.org changed reviewers: + rmcilroy@chromium.org
Please note I'm referencing results in Compile and Wasm benchmarks from https://codereview.chromium.org/2039233005/#ps1. This present CL is re-running those at the moment, too.
One suggestion, otherwise LGTM, thanks for the cleanup. https://codereview.chromium.org/2044283003/diff/1/src/arm/assembler-arm.cc File src/arm/assembler-arm.cc (right): https://codereview.chromium.org/2044283003/diff/1/src/arm/assembler-arm.cc#ne... src/arm/assembler-arm.cc:3946: pending_32_bit_constants_.reserve(kMaxNumPending32Constants); Do we need this branch (and kMinNumPendingConstants) any longer now that we only have a single resizable buffer? I think we can get rid of both and just rely on vector resizing appropraitely (after the initial reserve in the constructor), WDYT? https://codereview.chromium.org/2044283003/diff/1/src/arm/assembler-arm.cc#ne... src/arm/assembler-arm.cc:3967: } else if (pending_64_bit_constants_.size() == kMinNumPendingConstants) { ditto.
https://codereview.chromium.org/2044283003/diff/1/src/arm/assembler-arm.cc File src/arm/assembler-arm.cc (right): https://codereview.chromium.org/2044283003/diff/1/src/arm/assembler-arm.cc#ne... src/arm/assembler-arm.cc:3946: pending_32_bit_constants_.reserve(kMaxNumPending32Constants); On 2016/06/08 15:55:18, rmcilroy wrote: > Do we need this branch (and kMinNumPendingConstants) any longer now that we only > have a single resizable buffer? I think we can get rid of both and just rely on > vector resizing appropraitely (after the initial reserve in the constructor), > WDYT? I thought there might have been some heuristic for jumping from kMinNumPendingConstants all the way to kMaxNumPending{etc}. If that's not the case, absolutely, let's remove it. I'll do that and re-run Compile and Wasm, that should clear any concerns. On that, I'm noticing that all we'd be using kMaxNumPending{32|64}Constants for is DCHECKs. Any reason to keep those constants and the DCHECKs around? https://codereview.chromium.org/2044283003/diff/1/src/arm/assembler-arm.cc#ne... src/arm/assembler-arm.cc:3967: } else if (pending_64_bit_constants_.size() == kMinNumPendingConstants) { On 2016/06/08 15:55:18, rmcilroy wrote: > ditto. Acknowledged.
https://codereview.chromium.org/2044283003/diff/1/src/arm/assembler-arm.cc File src/arm/assembler-arm.cc (right): https://codereview.chromium.org/2044283003/diff/1/src/arm/assembler-arm.cc#ne... src/arm/assembler-arm.cc:3946: pending_32_bit_constants_.reserve(kMaxNumPending32Constants); On 2016/06/08 16:04:48, Mircea Trofin wrote: > On 2016/06/08 15:55:18, rmcilroy wrote: > > Do we need this branch (and kMinNumPendingConstants) any longer now that we > only > > have a single resizable buffer? I think we can get rid of both and just rely > on > > vector resizing appropraitely (after the initial reserve in the constructor), > > WDYT? > > I thought there might have been some heuristic for jumping from > kMinNumPendingConstants all the way to kMaxNumPending{etc}. If that's not the > case, absolutely, let's remove it. I'll do that and re-run Compile and Wasm, > that should clear any concerns. My guess would be this was just to keep the dynamic resizing simple since it was being done manually. Thanks for re-running. > On that, I'm noticing that all we'd be using kMaxNumPending{32|64}Constants for > is DCHECKs. Any reason to > keep those constants and the DCHECKs around? We should never have more than kMaxNumPending{32|64}Constants otherwise the pool would be larger than we could reach with pc relative addressing (see [1]), so I think it's still useful having those DHECKS. [1] https://cs.chromium.org/chromium/src/v8/src/arm/assembler-arm.h?q=kMaxNumPend...
https://codereview.chromium.org/2044283003/diff/1/src/arm/assembler-arm.cc File src/arm/assembler-arm.cc (right): https://codereview.chromium.org/2044283003/diff/1/src/arm/assembler-arm.cc#ne... src/arm/assembler-arm.cc:3946: pending_32_bit_constants_.reserve(kMaxNumPending32Constants); On 2016/06/08 16:14:29, rmcilroy wrote: > On 2016/06/08 16:04:48, Mircea Trofin wrote: > > On 2016/06/08 15:55:18, rmcilroy wrote: > > > Do we need this branch (and kMinNumPendingConstants) any longer now that we > > only > > > have a single resizable buffer? I think we can get rid of both and just rely > > on > > > vector resizing appropraitely (after the initial reserve in the > constructor), > > > WDYT? > > > > I thought there might have been some heuristic for jumping from > > kMinNumPendingConstants all the way to kMaxNumPending{etc}. If that's not the > > case, absolutely, let's remove it. I'll do that and re-run Compile and Wasm, > > that should clear any concerns. > > My guess would be this was just to keep the dynamic resizing simple since it was > being done manually. Thanks for re-running. > > > On that, I'm noticing that all we'd be using kMaxNumPending{32|64}Constants > for > > is DCHECKs. Any reason to > > keep those constants and the DCHECKs around? > > We should never have more than kMaxNumPending{32|64}Constants otherwise the pool > would be larger than we could reach with pc relative addressing (see [1]), so I > think it's still useful having those DHECKS. > > [1] > https://cs.chromium.org/chromium/src/v8/src/arm/assembler-arm.h?q=kMaxNumPend... I see - because we trigger constant pool generation based on the pc distance, the assertions need to also hold.
The CQ bit was checked by mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/2044283003/#ps20001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2044283003/20001
Message was sent while issue was closed.
Description was changed from ========== Use standard datastructures for tracking constant pool entries. This improves maintainability. The Compile and Wasm benchmarks, tracking compile time, show no regression. BUG= ========== to ========== Use standard datastructures for tracking constant pool entries. This improves maintainability. The Compile and Wasm benchmarks, tracking compile time, show no regression. BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use standard datastructures for tracking constant pool entries. This improves maintainability. The Compile and Wasm benchmarks, tracking compile time, show no regression. BUG= ========== to ========== Use standard datastructures for tracking constant pool entries. This improves maintainability. The Compile and Wasm benchmarks, tracking compile time, show no regression. BUG= Committed: https://crrev.com/88a92c47f87690772a5dbede71a103a801a39b94 Cr-Commit-Position: refs/heads/master@{#36834} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/88a92c47f87690772a5dbede71a103a801a39b94 Cr-Commit-Position: refs/heads/master@{#36834}
Message was sent while issue was closed.
FYI: This breaks a layout test with ignition: https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064%20-...
Message was sent while issue was closed.
On 2016/06/09 07:06:15, Michael Achenbach wrote: > FYI: This breaks a layout test with ignition: > https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064%20-... I don't think this CL caused this failure. This CL should only effect Arm, and that builder is x64 AFAIK. The test was updated recently, so it was probably just coincidence that this Chromium roll pulled in this update at the same time as Mircea CL. I'll update TestExpectations and file a bug. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
