Make it possible to extend the size of the heap #3

rafalmiel · 2017-04-25T21:36:09Z

Add a max_size parameter to the heap.
Allocations exceeding the size of the heap will fail,
but one can extend the size of the heap with the call to extend.

That allows heap to be more memory efficient, consuming more memory only
when necessary, eg. os allocator might map additional virtual memory when
the allocation fails.

oli-obk · 2017-04-25T22:45:12Z

How is this helpful without some callback asking whether the memory can be extended? I mean the memory can't be used according to the docs, so why not simply add it to the heap from the beginning?

rafalmiel · 2017-04-25T23:45:38Z

The intention was that the extend would be called by the user of the heap after allocating additional memory for the new extended heap. I would like to have initial small (bottom, size) area and when the allocation fails I allocate additional virtual address space at the end. Otherwise I would have to provide memory for (bottom, max_size) from the beginning, which might be wasteful if there will not be many allocations. Think of it as a sbrk call on linux

oli-obk · 2017-04-26T09:06:21Z

Ah, yea that makes sense. I got the API backwards in my head.

phil-opp

Thanks a lot for the PR! Overall, I like the idea of lazily extending the heap size.

It has a large overlap with demand mapping though, especially with the current first fit allocation policy: Instead of creating a small heap and lazily extending its size, one could also create a max_size heap but only map the first few virtual pages to physical memory. When these first few pages are full, a page fault occurs. The OS can then map additional virtual pages to physical memory and restart the operation. This has the big advantage that it is transparent to the application, i.e. the allocation never fails until the complete heap (of size max_size) is full. However, demand mapping also has disadvantages (e.g. doesn't work with best/worst fit policies, requires interrupt handling, makes performance less predictable, …), so I think it's fine to have both possibilities.

I added some comments on the implementation, especially about the increased API surface through the new max_size field and the HoleList::extend/put_hole methods.

phil-opp · 2017-04-26T15:58:10Z

src/hole.rs

@@ -40,6 +40,38 @@ impl HoleList {
        }
    }

+    /// Places a hole of a given size at a given address after a given hole
+    unsafe fn put_hole(hole: &mut Hole, addr: usize, size: usize) {
+        mem::forget(mem::replace(&mut *(addr as *mut Hole),


Why do we need mem::forget here?

Not sure if it is really needed here, but isn't it the simillar case as in the new call? It seems to work without the forget call though(because Hole does not have a drop Trait?). Still exploring the mysteries of rust :)

because Hole does not have a drop Trait?

Yes, exactly. I'm not sure why I used it in new… maybe Hole had a destructor back then and I forgot to remove it later. In any case, it shouldn't be needed here.

phil-opp · 2017-04-26T16:03:53Z

src/hole.rs

+
+    /// Extends the size of the list by increasing the size of the last hole
+    /// or appending new hole at the end of the list
+    pub fn extend(&mut self, addr: usize, size: usize) {


Why do we need extend and put_hole? Can't we just use the deallocate function for this?

Deallocate should work indeed, good catch. Perhaps it's just not a good name for this functionality? How about extend calling deallocate, or should we just get rid of extend call altogether?

How about extend calling deallocate, or should we just get rid of extend call altogether?

I'm fine with both variants.

phil-opp · 2017-04-26T16:10:34Z

src/lib.rs

@@ -17,6 +17,7 @@ mod test;
 pub struct Heap {
    bottom: usize,
    size: usize,
+    max_size: usize,


For what do we need max_size? The only reason I can see is that this makes it possible to make extend safe. Maybe it's better to have an unsafe extend function and let the caller manage max_size itself.

(Maybe we should even get rid of the bottom and size fields… Then we could expand the heap with an arbitrary memory chunk without the need for a single continuous block.)

Thanks for the review! Yes, that makes sense, we could leave it to the user to manage the complexity of the heap size. Also, one can then choose between allocating small heap size or going for on demand mapping. I will post the revised version in the coming days. Cheers

Thanks, looking forward to it! :)

rafalmiel · 2017-05-03T21:57:28Z

Hello,

I updated the pull request. As suggested I removed the max_size field and made the call to the extend unsafe. Also I make use of the deallocate method now (fixed some issue there. If the new hole was added just behind the last hole, the last hole should be merged, instead new hole was being created).

I hope changes looks better now :)

I have also separate branch where I removed bottom and size fields completely, but feel like it's a big and potentially breaking change for client code so wanted to keep it separate.

You can have a look into it here
rafalmiel@28764f4
if you wish I will also include it in the pull request.

Thanks!

phil-opp · 2017-05-09T17:13:20Z

Thanks a lot for the updates! Looks really good now.

I'm a bit short of time at the moment, but I try to take a closer look in the next few days.

rafalmiel · 2017-05-09T17:44:45Z

Thanks a lot, no need to hurry :)

phil-opp

I left a small improvement suggestion, but otherwise this looks good to me!

phil-opp · 2017-05-12T09:45:02Z

src/hole.rs

-                hole.next = Some(unsafe { Unique::new(ptr) });
+                if hole_addr + hole.size == addr {
+                    // after:   ___XXXFFFF___    where F is the freed block
+                    hole.size += size;


Good catch! I think we could merge this case with lines 243-249 by replacing the Some(_) with a None. Or am I missing something?

rafalmiel · 2017-05-21T19:58:03Z

Good catch! I think we could merge this case with lines 243-249 by replacing the Some(_) with a None. Or am I missing something?

Hi Phil,

I gave it a second thought and updated the pull request, but instead of None I went with _. I think if we do None, then we will miss the case when the new hole aligns to previous hole, but there are more holes later on the heap?

I hope it looks good, please let me know if I can improve it more.

Thanks,
Rafal

phil-opp

Thanks for the update!

instead of None I went with _. I think if we do None, then we will miss the case when the new hole aligns to previous hole, but there are more holes later on the heap?

Yeah, I meant to write _, sorry.

I hope it looks good, please let me know if I can improve it more.

I left a comment about the changed order of match cases. Other than that, I think this is ready to land :).

phil-opp · 2017-05-28T13:38:15Z

src/hole.rs

@@ -264,6 +257,17 @@ fn deallocate(mut hole: &mut Hole, addr: usize, mut size: usize) {
                hole = move_helper(hole).next_unwrap(); // start next iteration at next hole
                continue;
            }
+            _ if hole_addr + hole.size == addr => {


Is there a reason why you moved this match case to the bottom? I think it should work too, but I'm not 100% sure…

(The order matters for some cases, e.g. the XXXFFFFYYYYY case must come before the XXXFFFF__YYYYY and XXX__FFFFYYYYY cases.)

No big reason, just wanted to keep it close to the other _ case, but I am happy to move it back
Thanks!

Ok, then I would move it back

Add extend call that will increase the size of the heap by the given size. This is unsafe call and the caller must ensure that the new area is valid. That allows heap to be more memory efficient, consuming more memory only when necessary, eg. os allocator might map additional virtual memory when the allocation fails.

rafalmiel · 2017-06-09T19:44:45Z

Hi, I updated the pull request with the requested change to the match statement

phil-opp · 2017-06-11T16:01:02Z

Thanks a lot!

phil-opp · 2017-06-20T16:17:22Z

Published as version 0.2.7

phil-opp reviewed Apr 26, 2017

View reviewed changes

rafalmiel force-pushed the heap_extend branch from 3a25d3d to 4478299 Compare May 3, 2017 21:50

rafalmiel force-pushed the heap_extend branch from 4478299 to 420a993 Compare May 3, 2017 22:08

phil-opp reviewed May 12, 2017

View reviewed changes

rafalmiel force-pushed the heap_extend branch from 420a993 to a82900d Compare May 21, 2017 19:52

rafalmiel force-pushed the heap_extend branch 2 times, most recently from a68d697 to df307ec Compare May 21, 2017 20:57

phil-opp reviewed May 28, 2017

View reviewed changes

rafalmiel added 3 commits June 9, 2017 20:41

Increase the size of the last hole if the new hole aligns to it

8f8f585

Remove unnecessary mem::forget calls

f37bbaa

rafalmiel force-pushed the heap_extend branch from df307ec to 24db8fd Compare June 9, 2017 19:42

phil-opp merged commit 7dc809f into rust-osdev:master Jun 11, 2017

Make it possible to extend the size of the heap #3

Make it possible to extend the size of the heap #3

Uh oh!

Conversation

rafalmiel commented Apr 25, 2017

Uh oh!

oli-obk commented Apr 25, 2017

Uh oh!

rafalmiel commented Apr 25, 2017

Uh oh!

oli-obk commented Apr 26, 2017

Uh oh!

phil-opp left a comment

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!

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!

rafalmiel commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phil-opp commented May 9, 2017

Uh oh!

rafalmiel commented May 9, 2017

Uh oh!

phil-opp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafalmiel commented May 21, 2017

Uh oh!

phil-opp left a comment

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!

rafalmiel commented Jun 9, 2017

Uh oh!

phil-opp commented Jun 11, 2017

Uh oh!

phil-opp commented Jun 20, 2017

Uh oh!

Uh oh!

rafalmiel commented May 3, 2017 •

edited

Loading