RTS: Self-contained heap representation for bignums by nomeata · Pull Request #2280 · caffeinelabs/motoko

nomeata · 2021-01-29T10:23:29Z

To simplify GCs, we store the libtommath bignums as self-contained
objects on the heap (so for the purposes of the GC, they are just data).

The idea is that the TAG_BIGINT object stores both the mp_int record
and the mp_digit * array. See rts/motoko-rts/src/bigint.rs for more
details.

To simplify GCs, we store the libtommath bignums as self-contained objects on the heap (so for the purposes of the GC, they are just data). The idea is that the `TAG_BIGINT` object stores both the `mp_int` record and the `mp_digit *` array. See `rts/motoko-rts/src/bigint.rs` for more details.

dfinity-ci · 2021-01-29T11:06:47Z

In terms of gas, 1 tests regressed, 2 tests improved and the mean change is -1.4%.
In terms of size, 3 tests regressed and the mean change is +0.4%.

rts/motoko-rts/src/bigint.rs

crusso · 2021-01-29T12:09:07Z

rts/motoko-rts/src/bigint.rs

 #[no_mangle]
 pub unsafe extern "C" fn bigint_add(a: SkewedPtr, b: SkewedPtr) -> SkewedPtr {
-    let r = bigint_alloc();
+    let mut i = tmp_bigint();


There seems to be a pattern here of alloc then persist - @osa1 would it be just as efficient, bug safer, to use a higher-order function here like your with_mp_int(f) functions?

(I may be mis-remembering the name)

I was worried about the compiler not reliably inlining a with_mp_int function, and suddenly function pointers being passed around, so I shied away from it and wrote the code I expect to see in the wasm. But maybe I should give it a try… is there a way to guarantee that it will be inlined?

Yeah, I was worried about that too with with_mp_int, but @osa1 proved me wrong by looking at the generated code. But probably not worth the hassle TBH. Also, I think you've got one or two place that don't follow the pattern - not every tmp_BigInt is persisted - or is that maybe a bug?

No, not a bug, these are indeed temporary (as the same says).

I think it's fine: not every bigint_alloc needs to be persisted, and the types ensure that you don't forget to call persist if you mean to use it. So we'd be trading two simple functions for two(?) higher order functions.

OK, as long the GC can safely traverse and collect the unpersisted ones.
The trade-off is less about the number of functions but the number of calls you need to make (or could forget to make) at each use site, I think. But I'm ok with either approach.

The unpersited mp_int lives on the heap. The allocated thing on the heap is safe (and must be even without persist, e.g. for the tmp values)

You can't forget, the type won't allow it. Else I'd agree that a safer idiom might be advisable.

crusso

Looks promising, but @osa1 is the one to review properly

Co-authored-by: Claudio Russo <claudio@dfinity.org>

crusso · 2021-02-02T22:19:45Z

If osa is MIA, I'm happy to take another look and approve if you are confident.

nomeata · 2021-02-03T08:45:29Z

Confident enough, with a second look from you

rts/motoko-rts/src/bigint.rs

crusso · 2021-02-03T12:08:39Z

rts/motoko-rts/src/bigint.rs

 unsafe extern "C" fn mp_calloc(n_elems: usize, elem_size: Bytes<usize>) -> *mut libc::c_void {
+    debug_assert_eq!(elem_size.0, core::mem::size_of::<mp_digit>());
    let size = Bytes((n_elems * elem_size.0) as u32); // Overflow check?
    let payload = mp_alloc(size) as *mut u32;


Can the zeroing loop below be done more efficiently with a memset or something?

I’ll check if the compiler does something smart. But memset itself can’t do more than write a word of zero at a time.

crusso · 2021-02-03T12:20:56Z

rts/motoko-rts/src/bigint.rs

 }

 #[no_mangle]
 unsafe extern "C" fn bigint_div(a: SkewedPtr, b: SkewedPtr) -> SkewedPtr {


Aside: Do we have divrem that returns both at once?

Nope, not yet

rts/motoko-rts/src/types.rs

crusso · 2021-02-03T12:39:38Z

rts/motoko-rts/src/types.rs

+    /// libtommath function that tries to change it. For example, we cannot confuse input and
+    /// output paramters of mp_add() this way.
+    pub unsafe fn mp_int_ptr(self: *mut BigInt) -> *const mp_int {
+        (*self).mp_int.dp = self.payload_addr();


would avoiding the fix up dirty fewer pages? Can this just be done during evacuation?

It could be done there, but I guess a point of this exercise was to make the GC oblivious of the bigint stuff. And this is safer. So not sure.

crusso

But see comments - will leave changes up to you

Co-authored-by: Claudio Russo <claudio@dfinity.org>

…m-object

nomeata requested a review from osa1 January 29, 2021 10:24

nomeata mentioned this pull request Jan 29, 2021

New GC: in-place compaction #2223

Closed

1 task

crusso reviewed Jan 29, 2021

View reviewed changes

rts/motoko-rts/src/bigint.rs Outdated Show resolved Hide resolved

crusso reviewed Jan 29, 2021

View reviewed changes

rts/motoko-rts/src/bigint.rs Show resolved Hide resolved

crusso reviewed Jan 29, 2021

View reviewed changes

nomeata and others added 3 commits January 29, 2021 13:20

Inline alloc_bigint into mp_alloc

7db2301

Use debug_assert_eq

b710b9c

Apply suggestions from code review

944a772

Co-authored-by: Claudio Russo <claudio@dfinity.org>

crusso reviewed Feb 3, 2021

View reviewed changes

rts/motoko-rts/src/bigint.rs Outdated Show resolved Hide resolved

crusso reviewed Feb 3, 2021

View reviewed changes

rts/motoko-rts/src/types.rs Outdated Show resolved Hide resolved

crusso reviewed Feb 3, 2021

View reviewed changes

rts/motoko-rts/src/types.rs Outdated Show resolved Hide resolved

crusso reviewed Feb 3, 2021

View reviewed changes

crusso approved these changes Feb 3, 2021

View reviewed changes

nomeata and others added 5 commits February 3, 2021 13:47

Update rts/motoko-rts/src/types.rs

4894d1b

Co-authored-by: Claudio Russo <claudio@dfinity.org>

Update rts/motoko-rts/src/types.rs

13f4cf6

Co-authored-by: Claudio Russo <claudio@dfinity.org>

Check overflow in mp_calloc

af74025

Merge branch 'master' of github.com:dfinity/motoko into joachim/bignu…

80ff5b3

…m-object

Cargo-fmt

9747961

nomeata added the automerge-squash When ready, merge (using squash) label Feb 3, 2021

mergify bot merged commit f9112c8 into master Feb 3, 2021

mergify bot deleted the joachim/bignum-object branch February 3, 2021 14:14

mergify bot removed the automerge-squash When ready, merge (using squash) label Feb 3, 2021

Comments

Conversation

nomeata commented Jan 29, 2021

Uh oh!

dfinity-ci commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso left a comment

Choose a reason for hiding this comment

Uh oh!

crusso commented Feb 2, 2021

Uh oh!

nomeata commented Feb 3, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nomeata Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dfinity-ci commented Jan 29, 2021 •

edited

Loading

nomeata Feb 3, 2021 •

edited

Loading