Call Collection::out_of_memory if the allocation size is larger than max heap size

[qinsoon]

This closes #867.

[wks]

See in-line comments. We probably can't let the VM recover from OOM for now. For the test case, maybe catch_unwind can be used to prevent the lock from being poisoned.

[qinsoon]

There is an issue with this PR: if we try to do any address arithmetic in the allocation fastpath, we may have overflow in the computation, and we do not call out_of_memory() for that case. See the test below. I don't think we have a good way to deal with this, given that we do not want to introduce any check in the fastpath.

mmtk-core/vmbindings/dummyvm/src/tests/issue867_allocate_unrealistically_large_object.rs

Line 27 in 04c0c48

pub fn allocate_max_size_object_after_succeed() {

[wks]

LGTM

[k-sareen]

There is an issue with this PR: if we try to do any address arithmetic in the allocation fastpath, we may have overflow in the computation, and we do not call out_of_memory() for that case. See the test below. I don't think we have a good way to deal with this, given that we do not want to introduce any check in the fastpath.

mmtk-core/vmbindings/dummyvm/src/tests/issue867_allocate_unrealistically_large_object.rs

Line 27 in 04c0c48

pub fn allocate_max_size_object_after_succeed() {

One alternative, although I don't know how feasible it is, would be to register a custom panic handler and then call either out_of_memory or panic_in_vm (random function) or something to allow the VM binding to crash by itself. We discussed doing something similar in the OOM PR and #746

[wks]

There is an issue with this PR: if we try to do any address arithmetic in the allocation fastpath, we may have overflow in the computation, and we do not call out_of_memory() for that case. See the test below. I don't think we have a good way to deal with this, given that we do not want to introduce any check in the fastpath.

mmtk-core/vmbindings/dummyvm/src/tests/issue867_allocate_unrealistically_large_object.rs

Line 27 in 04c0c48

pub fn allocate_max_size_object_after_succeed() {

I think doing checking in Rust is OK. The "real" fast path should be implemented by the JIT compiler, or inlined to the concrete allocation sites. In most of the cases, the object size should be a constant, and doesn't need checking anyway, given that a bump-pointer allocator always checks against a reaonably-sized limit. If the object size is a variable, the VM should do the check anyway to avoid integer overflow. If the VM simply calls MMTk API function for fast-path allocation, it means the VM has not started optimising for performance (like the status of the Ruby binding two months ago). In that case, we should prefer correctness over performance.

[k-sareen]

If the VM simply calls MMTk API function for fast-path allocation, it means the VM has not started optimising for performance

In the case of a VM implemented in Rust, this is not necessarily true. It would be inlined by PGO

[wks]

If the VM simply calls MMTk API function for fast-path allocation, it means the VM has not started optimising for performance

In the case of a VM implemented in Rust, this is not necessarily true. It would be inlined by PGO

That makes sense. But if that's the case, the compiler can still eliminate the object size check if the supplied object size is a constant, although I am not 100% confident that the compiler will do it.

[k-sareen]

Often a "checked" and "unchecked" API is presented to a developer. They are recommended to use the "checked" one always but they can use the "unchecked" one for more performance, but they have to guarantee that they have checked the assumptions beforehand

[k-sareen]

Should we merge this or discuss how to fix the bug Yi noticed?

[qinsoon]

Should we merge this or discuss how to fix the bug Yi noticed?

I think we can merge this. I will try out the idea of catching Rust panics in a separate PR (I cannot guarantee it would work out though).