Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556483002:
[promises] allow allocation in large object space (Closed)
Description[promises] allow allocation in large object space
BUG=v8:5343
Committed: https://crrev.com/05b6741f0184662542487c3982b64beba587135b
Cr-Commit-Position: refs/heads/master@{#41538}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : use AllowLargeObjectSpace flag #Patch Set 4 : use var #Messages
Total messages: 28 (18 generated)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [promises] use runtime to allocate larger arrays BUG=v8:5343 ========== to ========== [promises] use runtime to allocate larger arrays BUG=v8:5343 ==========
gsathya@chromium.org changed reviewers: + bmeurer@chromium.org, caitp@igalia.com, ishell@google.com, jgruber@chromium.org
On 2016/12/05 21:25:13, gsathya wrote: What if we just added an option to the normal CSA::Allocate() to test if the object size needs to be allocated in lospace, even if the object isn't pretenured normally?
On 2016/12/05 21:37:36, caitp wrote: > On 2016/12/05 21:25:13, gsathya wrote: > > What if we just added an option to the normal CSA::Allocate() to test if the > object size needs to be allocated in lospace, even if the object isn't > pretenured normally? and that's if we actually care about this, since it does seem unlikely for this to happen in real code
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/05 21:38:03, caitp wrote: > On 2016/12/05 21:37:36, caitp wrote: > > On 2016/12/05 21:25:13, gsathya wrote: > > > > What if we just added an option to the normal CSA::Allocate() to test if the > > object size needs to be allocated in lospace, even if the object isn't > > pretenured normally? > > and that's if we actually care about this, since it does seem unlikely for this > to happen in real code I don't know, I feel like AllocateFixedArray is a low level operation that shouldn't worry about bailing out or calling out to the runtime. All the abstractions over it(TryGrow..) deal with the bailing out themselves. Don't feel too strongly about this though
On 2016/12/05 21:53:25, gsathya wrote: > On 2016/12/05 21:38:03, caitp wrote: > > On 2016/12/05 21:37:36, caitp wrote: > > > On 2016/12/05 21:25:13, gsathya wrote: > > > > > > What if we just added an option to the normal CSA::Allocate() to test if the > > > object size needs to be allocated in lospace, even if the object isn't > > > pretenured normally? > > > > and that's if we actually care about this, since it does seem unlikely for > this > > to happen in real code At least for the RegExp case (which constructs FixedArrays to store results in @@match and @@split), this does seem to happen in real code, see crbug.com/670575. > I don't know, I feel like AllocateFixedArray is a low level operation that > shouldn't worry about bailing out or calling out to the runtime. All the > abstractions over it(TryGrow..) deal with the bailing out themselves. I briefly discussed with Igor today and I'm working on a CL to add a new kAllowLargeObjectSpace flag to CSA::AllocationFlags.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [promises] use runtime to allocate larger arrays BUG=v8:5343 ========== to ========== [promises] allow allocation in large object space BUG=v8:5343 ==========
On 2016/12/06 11:38:59, jgruber wrote: > On 2016/12/05 21:53:25, gsathya wrote: > > On 2016/12/05 21:38:03, caitp wrote: > > > On 2016/12/05 21:37:36, caitp wrote: > > > > On 2016/12/05 21:25:13, gsathya wrote: > > > > > > > > What if we just added an option to the normal CSA::Allocate() to test if > the > > > > object size needs to be allocated in lospace, even if the object isn't > > > > pretenured normally? > > > > > > and that's if we actually care about this, since it does seem unlikely for > > this > > > to happen in real code > > At least for the RegExp case (which constructs FixedArrays to store results in > @@match and @@split), this does seem to happen in real code, see > crbug.com/670575. > > > I don't know, I feel like AllocateFixedArray is a low level operation that > > shouldn't worry about bailing out or calling out to the runtime. All the > > abstractions over it(TryGrow..) deal with the bailing out themselves. > > I briefly discussed with Igor today and I'm working on a CL to add a new > kAllowLargeObjectSpace flag to CSA::AllocationFlags. PTAL, updated CL to use the kAllowLargeObjectSpace flag.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ishell@chromium.org changed reviewers: + ishell@chromium.org
lgtm
The CQ bit was checked by gsathya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481059759072190,
"parent_rev": "60070a31cd7528f8235272382d49122a8eab580b", "commit_rev":
"31439d31a0c84cd902c04bac4c8afc1d037eb182"}
Message was sent while issue was closed.
Description was changed from ========== [promises] allow allocation in large object space BUG=v8:5343 ========== to ========== [promises] allow allocation in large object space BUG=v8:5343 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [promises] allow allocation in large object space BUG=v8:5343 ========== to ========== [promises] allow allocation in large object space BUG=v8:5343 Committed: https://crrev.com/05b6741f0184662542487c3982b64beba587135b Cr-Commit-Position: refs/heads/master@{#41538} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/05b6741f0184662542487c3982b64beba587135b Cr-Commit-Position: refs/heads/master@{#41538} |
