Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Better allocator for wasm #1460

Merged
pepyakin merged 25 commits into from
Jan 18, 2019
Merged

Better allocator for wasm #1460

Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
2741554
Add Arch Linux installation instructions
cmichi Jan 14, 2019
e95140a
Enable tracing heap size
gavofyork Jan 11, 2019
d2a7935
Extract heap
cmichi Jan 12, 2019
aff276a
Replace linear allocator with buddy allocator
cmichi Jan 14, 2019
292c177
Fix test
cmichi Jan 16, 2019
1d71e15
Get rid of memcpy in to_vec()
cmichi Jan 16, 2019
b65f0e4
fixup: Style and comments
cmichi Jan 17, 2019
02a3d1e
fixup: Split Linux instructions by distribution
cmichi Jan 17, 2019
dfa62a8
fixup: Remove unnecessary types and code
cmichi Jan 17, 2019
41e4ab7
fixup: Make Pointers start from 1, remove some panics, code improvements
cmichi Jan 17, 2019
53bf828
fixup: Return 0 on errors
cmichi Jan 17, 2019
b189736
fixup: Move loop to separate function
cmichi Jan 17, 2019
18a1efc
fixup: Use FnvHashMap instead of HashMap
cmichi Jan 17, 2019
137b5bd
fixup: Fix error handling
cmichi Jan 17, 2019
741e972
fixup: Use current_size() instead of used_size()
cmichi Jan 17, 2019
93dc2e9
fixup: Fix and document allocation offset
cmichi Jan 17, 2019
d4676b0
fixup: Remove unnecessary multiplication
cmichi Jan 17, 2019
6a99426
fixup: Fix comments
cmichi Jan 17, 2019
6725a49
fixup: Remove Arch installation instructions
cmichi Jan 17, 2019
6438db3
Revert "Fix test"
cmichi Jan 17, 2019
ecd9731
fixup: Remove unused code, improve import
cmichi Jan 17, 2019
ca065b1
fixup: Proper alignment
cmichi Jan 17, 2019
58ee588
fixup: Do not use internal constant in public description
cmichi Jan 17, 2019
247d37b
fixup: Add comment regarding invariants
cmichi Jan 18, 2019
8e4deda
fixup: Move assertion to compile-time check
cmichi Jan 18, 2019
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
Prev Previous commit
Next Next commit
fixup: Add comment regarding invariants
  • Loading branch information
@cmichi
cmichi committed Jan 18, 2019
commit 247d37b58367f52db27022c506025e9bb911670a
10 changes: 7 additions & 3 deletions core/sr-io/without_std.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ pub fn storage(key: &[u8]) -> Option<Vec<u8>> {
if length == u32::max_value() {
None
} else {
// Invariants required by Vec::from_raw_parts are not formally fulfilled.
// We don't allocate via String/Vec<T>, but use a custom allocator instead.
// See #300 for more details.
Some(<Vec<u8>>::from_raw_parts(ptr, length as usize, length as usize))
Copy link
Contributor

@pepyakin pepyakin Jan 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

As was discussed in the issue, this actually violates formal preconditions required by Vec::from_raw_parts (i.e. the pointer is allocated not with Vec routines). Given that we agree with it, I still would like to have a comment here mentioning this fact.

And there should be another case nearby (below).

Copy link
Member

@bkchr bkchr Jan 17, 2019

Choose a reason for hiding this comment

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

I'm still against using Vec::from_raw_parts because of all the invariants. If we can prove the invariants, I'm happy.

Copy link
Contributor Author

@cmichi cmichi Jan 17, 2019

Choose a reason for hiding this comment

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

@gavofyork What is your opinion on this?

Copy link
Contributor

@rphmeier rphmeier Jan 17, 2019

Choose a reason for hiding this comment

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

It may be better to return some other new type as well.

Copy link
Member

@gavofyork gavofyork Jan 17, 2019

Choose a reason for hiding this comment

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

As I stated in the other thread I'm against this UB dogma. I demonstrated that this code is safe. If you think otherwise please bring the argument forward, otherwise this code goes in.

Copy link
Contributor

@pepyakin pepyakin Jan 18, 2019

Choose a reason for hiding this comment

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

Yes, I'd agree that it would work for us and I don't have anymore to add to our discussion here! But I'd still be in favor of a comment here. I hope it will save us a headache.

Also, @cmichi there is another case where we apply the same pattern: child_storage, a function below. I'd like them to be synchronized.

}
}
Expand All @@ -145,9 +148,10 @@ pub fn child_storage(storage_key: &[u8], key: &[u8]) -> Option<Vec<u8>> {
if length == u32::max_value() {
None
} else {
let ret = slice::from_raw_parts(ptr, length as usize).to_vec();
ext_free(ptr);
Some(ret)
// Invariants required by Vec::from_raw_parts are not formally fulfilled.
// We don't allocate via String/Vec<T>, but use a custom allocator instead.
// See #300 for more details.
Some(<Vec<u8>>::from_raw_parts(ptr, length as usize, length as usize))
}
}
}
Expand Down