Skip to content

perf: optimize overflow check in RTS alloc_words, fix and update tests #3085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 31, 2022
Merged

perf: optimize overflow check in RTS alloc_words, fix and update tests #3085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ebf3483
optimize overflow check, fix and update tests
crusso Jan 29, 2022
381fb3a
refactor
crusso Jan 29, 2022
e26eb7e
formatting
crusso Jan 29, 2022
f6d70fb
remove redundant cast
crusso Jan 29, 2022
620f2de
Merge branch 'master' into claudio/oom-opt
crusso Jan 30, 2022
02e277b
Merge branch 'master' into claudio/oom-opt
crusso Jan 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions rts/motoko-rts/src/memory/ic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,29 +72,29 @@ impl Memory for IcMemory {
unsafe fn alloc_words(&mut self, n: Words<u32>) -> Value {
let bytes = n.to_bytes();
// Update ALLOCATED
ALLOCATED += Bytes(u64::from(bytes.as_u32()));
let delta = u64::from(bytes.as_u32());
ALLOCATED += Bytes(delta);

// Update heap pointer
let old_hp = HP;
let (new_hp, overflow) = old_hp.overflowing_add(bytes.as_u32());
// Check for overflow
if overflow {
rts_trap_with("Out of memory");
}
HP = new_hp;
let old_hp = u64::from(HP);
let new_hp = old_hp + delta;

// Grow memory if needed
grow_memory(new_hp as usize);
grow_memory(new_hp);
Copy link
Contributor

@ggreif ggreif Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ulan commenting remark in the meeting just now. The fast path should be predicated on the fact that the allocation remains on the last page, i.e. old_hp >> 16 == new_hp >> 16.

And that condition could even be made faster by (old_hp ^ new_hp) >> 16. See #3090.

Copy link
Contributor Author

@crusso crusso Jan 31, 2022 •
edited by ggreif
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but I'll leave that to #3090.


debug_assert!(new_hp <= u64::from(core::u32::MAX));
HP = new_hp as u32;

Value::from_ptr(old_hp as usize)
}
}

/// Page allocation. Ensures that the memory up to, but excluding, the given pointer is allocated.
#[inline(never)]
unsafe fn grow_memory(ptr: usize) {
unsafe fn grow_memory(ptr: u64) {
debug_assert!(ptr <= 2 * u64::from(core::u32::MAX));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use the same condition as in the assert in line 85: ptr <= u64::from(core::u32::MAX) (i.e. compare against 4GiB, not 8GiB)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'm deliberately comparing against 8GB because I want to test the overflow detection later on (otherwise we will get different faults on debug and release builds).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it. Sounds good!

let page_size = u64::from(WASM_PAGE_SIZE.as_u32());
let total_pages_needed = (((ptr as u64) + page_size - 1) / page_size) as usize;
let total_pages_needed = ((ptr + page_size - 1) / page_size) as usize;
let current_pages = wasm32::memory_size(0);
if total_pages_needed > current_pages {
if wasm32::memory_grow(0, total_pages_needed - current_pages) == core::usize::MAX {
Expand Down
2 changes: 1 addition & 1 deletion test/run-drun/ok/oom-query.drun-run.ok
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
ingress Completed: Reply: 0x4449444c0000
Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: RTS error: Out of memory
Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: RTS error: Cannot grow memory
2 changes: 1 addition & 1 deletion test/run-drun/ok/oom-update.drun-run.ok
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
ingress Completed: Reply: 0x4449444c0000
Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: RTS error: Out of memory
ingress Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: RTS error: Cannot grow memory
4 changes: 2 additions & 2 deletions test/run-drun/oom-update.mo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import P "mo:⛔";
actor {

public query func go() : async () {
public func go() : async () {
// allocate 3GB
var c = 3;
while(c > 0) {
Expand All @@ -25,5 +25,5 @@ actor {
//SKIP run-ir
// too slow on ic-ref-run:
//SKIP comp-ref
//CALL query go "DIDL\x00\x00"
//CALL ingress go "DIDL\x00\x00"

2 changes: 1 addition & 1 deletion test/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ function normalize () {
# Normalize instruction locations on traps, added by ic-ref ad6ea9e
sed 's/region:0x[0-9a-fA-F]\+-0x[0-9a-fA-F]\+/region:0xXXX-0xXXX/g' |
# Delete everything after Oom
sed '/RTS error: Out of memory/q' |
sed '/RTS error: Cannot grow memory/q' |
cat > $1.norm
mv $1.norm $1
fi
Expand Down
2 changes: 1 addition & 1 deletion test/run/ok/inc-oom.wasm-run.ok
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
RTS error: Out of memory
RTS error: Cannot grow memory