Skip to content
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

Jump to bottom

[WIP] Update median to support floats #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[WIP] Update median to support floats #6

wants to merge 1 commit into from

Conversation

binglekruger
Copy link
Contributor

@binglekruger binglekruger commented Aug 17, 2022 •
edited
Loading

Update median to support floating point numbers

TODO

billguo99
billguo99 reviewed Aug 17, 2022
View reviewed changes
Copy link
Contributor

@billguo99 billguo99 left a comment

Choose a reason for hiding this comment

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

https://www.conventionalcommits.org/en/v1.0.0/

Can use git commit --amend to fix git commit message and then use git push -f to force push local branch to remote branch.

I tried make fclean which cleans the Cargo lock files and removes all the binaries in bin/ and then make but encountered a build error. Do you get the same?

@binglekruger binglekruger requested a review from billguo99 August 22, 2022 09:09
@billguo99
Copy link
Contributor

These changes replaces ints with floats. Are we planning to keep support for ints?

@binglekruger
Copy link
Contributor Author

These changes replaces ints with floats. Are we planning to keep support for ints?

I think there are two approaches we can follow:

  • Two separate binaries for each operation (int & float) [Downside: Lots of binaries to manage]
  • Convert ints to floats when importing data [Downside: Most likely incur performance penalty of f64 vs int]

Let me know what your thoughts are.

@billguo99
Copy link
Contributor

I think we can follow two separate binaries for each operation.
Pros:

  • No performance penalties
  • Can go implement the other approach (convert ints to floats) if that's better
  • With how many operations we'll have, we'll have to manage lots of binaries to manage anyway
  • Can experiment more with more binaries to find an optimal solution

@sonasi
Copy link
Contributor

sonasi commented Aug 23, 2022

I think Bill is right that we need a separate implementation for both ints and floats, rather than converting int to float. Any time you are working with large data or limited memory, the difference between int and float is significant.

@binglekruger binglekruger force-pushed the median-float-update branch 2 times, most recently from 0b7353e to 7f3ae5f Compare August 29, 2022 12:42
@binglekruger
Copy link
Contributor Author

I increased the memory of the enclave [2936708 KB] and the memory in wasmi-impl to ~850 MB. I have issues increasing the memory of wasmi-impl further.

Currently working on importing data - we need to discuss this. [As this depends on sealing/unsealing and the format of the data - I assume it will be JSON].

@binglekruger binglekruger marked this pull request as ready for review August 31, 2022 11:30
billguo99
billguo99 reviewed Sep 5, 2022
View reviewed changes
enclave/src/lib.rs

let import_data: vec::Vec<f64> = vec![1.24,2.0,3.4,4.7,5.5,10.5];
let data_string = format!("{:?}",import_data);
let data_slice = data_string.as_str().as_bytes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we change from using the byte string literal to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was testing how we handle importing the data, it is not needed for the binary

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool cool.

wasmi-impl/src/lib.rs
@@ -1,6 +1,6 @@
#![no_std]
use wasmi::{
self, memory_units::Pages, ExternVal, ImportsBuilder, MemoryInstance, ModuleInstance,
self, memory_units::{Pages}, ExternVal, ImportsBuilder, MemoryInstance, ModuleInstance,
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: are these braces around Pages extra?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had another memory unit at some time {Pages, X}, not needed

wasmi-impl/src/lib.rs
@@ -24,7 +24,7 @@ pub fn exec_wasm_with_data(
let module = wasmi::Module::from_buffer(binary)?;

// TODO: Calculate the memory size always be larger than `data`
let mem_instance = MemoryInstance::alloc(Pages(200), None)?;
let mem_instance = MemoryInstance::alloc(Pages(13000), None)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 13000 an arbitrary number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kind of, it was the maximum size I could allocate without errors.

It is 13000*64 KiB memory, which is ~850 MB

The theoretical limit is 65536, (4GB) but I cannot allocate enough memory on my VM

We need to test it with more memory allocated in Enclave.config.xml

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a link to where the theoretical limit comes from?

A single unit of Pages is 65536 bytes I believe - https://docs.rs/memory_units/latest/memory_units/wasm32/struct.Pages.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry here you go: https://paritytech.github.io/wasmi/wasmi/struct.MemoryInstance.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok cool thanks.

@billguo99
Copy link
Contributor

Have you tried changing these I32 to F64?

wasm-exec-sgx/wasmi-impl/src/lib.rs

Line 39 in c2310a3

&[RuntimeValue::I32(0), RuntimeValue::I32(data.len() as i32)],

@binglekruger
Copy link
Contributor Author

Yeah, the second part should stay I32, but maybe @sonasi can look into the first part? (RuntimeValue::I32(0))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@billguo99 billguo99 billguo99 approved these changes

@jdvlio jdvlio Awaiting requested review from jdvlio

@sonasi sonasi Awaiting requested review from sonasi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@binglekruger @billguo99 @sonasi