This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Is this still a valid issue?

Maybe. 

Jakob, you probably have a good overview of allocation in CSA. Can you comment on whether this is (still) an issue?

CSA currently supports LO-space allocation, but callers need to explicitly pass the kAllowLargeObjectAllocation flag [0,1]:

* AllocateRaw guarantees new-space allocation by default, and crashes at runtime if the requested size exceeds kMaxRegularHeapObjectSize.
* Passing AllocationFlags::kPretenured allocates in old space, and likewise crashes at runtime for large objects.
* Passing AllocationFlags::kAllowLargeObjectAllocation checks whether LO-space allocation is required, and if so calls into runtime.

Unfortunately, with the current solution it's easy to miss cases where LOS allocation might be needed (see e.g. v8:6716). But OTOH, we can't enable kAllowLargeObjectAllocation by default since we often rely on the allocated object being in new-space (to skip write barriers).

I could imagine a solution in which CSA allocations become safe (= they include the LOS check) by default, and callers can opt-in to guaranteed new-space allocation, if needed. WDYT?

As an aside, both AllocateSeq{One,Two}ByteString (mentioned in #0) could be refactored to use that flag instead of hand-coding a runtime path.

[0] https://cs.chromium.org/chromium/src/v8/src/code-stub-assembler.h?l=83&rcl=07259a9ceafa078c9bb7f9ee1bb6f2d67256cc80
[1] https://cs.chromium.org/chromium/src/v8/src/code-stub-assembler.cc?l=808&rcl=07259a9ceafa078c9bb7f9ee1bb6f2d67256cc80

> I could imagine a solution in which CSA allocations become safe (= they include the LOS check) by default, and callers can opt-in to guaranteed new-space allocation, if needed. WDYT?

That sounds great; as I said offline the requirement should be a runtime failure, even in release mode. Anything better is obviously awesome! :)

On second thought, opt-in new-space allocation isn't a good idea since forgetting to opt-in could result in missed write barriers.

I think it's better to opt-in to a potential LO allocations.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/f6d735095f9c9328a18f46f2a3b602ca79560907

commit f6d735095f9c9328a18f46f2a3b602ca79560907
Author: jgruber <jgruber@chromium.org>
Date: Tue Aug 22 11:50:29 2017

[csa] Refactor large-object handling in string allocation

CSA::AllocateSeq{One,Two}ByteString used its own home-grown handling to
allocate very large strings. This CL refactors both methods to use
AllocationFlags::kAllowLargeObjectAllocation instead. Callers now need
to specify explicitly if large-object allocation is possible or not.

Bug: chromium:636391
Cq-Include-Trybots: master.tryserver.v8:v8_linux_noi18n_rel_ng
Change-Id: I0b7ffb0b083f4e977cea42c500f8f2ee1c60519f
Reviewed-on: https://chromium-review.googlesource.com/625738
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47504}
[modify] https://crrev.com/f6d735095f9c9328a18f46f2a3b602ca79560907/src/builtins/builtins-intl-gen.cc
[modify] https://crrev.com/f6d735095f9c9328a18f46f2a3b602ca79560907/src/builtins/builtins-regexp-gen.cc
[modify] https://crrev.com/f6d735095f9c9328a18f46f2a3b602ca79560907/src/builtins/builtins-string-gen.cc
[modify] https://crrev.com/f6d735095f9c9328a18f46f2a3b602ca79560907/src/code-stub-assembler.cc
[modify] https://crrev.com/f6d735095f9c9328a18f46f2a3b602ca79560907/src/code-stubs.cc
[modify] https://crrev.com/f6d735095f9c9328a18f46f2a3b602ca79560907/src/debug/debug-evaluate.cc
[modify] https://crrev.com/f6d735095f9c9328a18f46f2a3b602ca79560907/src/runtime/runtime-internal.cc
[modify] https://crrev.com/f6d735095f9c9328a18f46f2a3b602ca79560907/src/runtime/runtime.h

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/3168a963ff3a85c49f4e0ef9c4e93938c22d07e6

commit 3168a963ff3a85c49f4e0ef9c4e93938c22d07e6
Author: Jakob Gruber <jgruber@chromium.org>
Date: Mon Aug 28 11:02:27 2017

Revert "[csa] Refactor large-object handling in string allocation"

This reverts commit f6d735095f9c9328a18f46f2a3b602ca79560907.

Reason for revert: Perf regressions https://crbug.com/758126

Original change's description:
> [csa] Refactor large-object handling in string allocation
> 
> CSA::AllocateSeq{One,Two}ByteString used its own home-grown handling to
> allocate very large strings. This CL refactors both methods to use
> AllocationFlags::kAllowLargeObjectAllocation instead. Callers now need
> to specify explicitly if large-object allocation is possible or not.
> 
> Bug: chromium:636391
> Cq-Include-Trybots: master.tryserver.v8:v8_linux_noi18n_rel_ng
> Change-Id: I0b7ffb0b083f4e977cea42c500f8f2ee1c60519f
> Reviewed-on: https://chromium-review.googlesource.com/625738
> Reviewed-by: Camillo Bruni <cbruni@chromium.org>
> Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#47504}

TBR=cbruni@chromium.org,jgruber@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:636391
Change-Id: Iab88ce400f489a677df821d4053bd3678289ae2e
Cq-Include-Trybots: master.tryserver.v8:v8_linux_noi18n_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/637392
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47639}
[modify] https://crrev.com/3168a963ff3a85c49f4e0ef9c4e93938c22d07e6/src/builtins/builtins-intl-gen.cc
[modify] https://crrev.com/3168a963ff3a85c49f4e0ef9c4e93938c22d07e6/src/builtins/builtins-regexp-gen.cc
[modify] https://crrev.com/3168a963ff3a85c49f4e0ef9c4e93938c22d07e6/src/builtins/builtins-string-gen.cc
[modify] https://crrev.com/3168a963ff3a85c49f4e0ef9c4e93938c22d07e6/src/code-stub-assembler.cc
[modify] https://crrev.com/3168a963ff3a85c49f4e0ef9c4e93938c22d07e6/src/code-stubs.cc
[modify] https://crrev.com/3168a963ff3a85c49f4e0ef9c4e93938c22d07e6/src/debug/debug-evaluate.cc
[modify] https://crrev.com/3168a963ff3a85c49f4e0ef9c4e93938c22d07e6/src/runtime/runtime-internal.cc
[modify] https://crrev.com/3168a963ff3a85c49f4e0ef9c4e93938c22d07e6/src/runtime/runtime.h