port fiber changes from codal for PXT GC support #424

mmoskal · 2019-02-22T00:00:00Z

This is port of relevant codal changes in support of GC in the PXT runtime. Also, limit the number of fibers in pool to 3.

Also port from codal-core in support of PXT GC. Also, fix compiler optimization bug in calloc().

abchatra · 2019-02-27T22:24:06Z

@mmoskal Can you please merge this? Also if possible create the right tags in the repos so we can point to it.

finneyj · 2019-02-27T22:55:44Z

@mmoskal @abchatra

Would you mind holding off on merging until I've had chance to review the changes to the scheduler fully? Some changes in this PR are pretty deep, and I feel another pair of eyes on it can only help. I'll try to find time in the next 24hrs...

abchatra · 2019-02-27T22:58:36Z

Sure @finneyj. Sorry, I thought review was completed. (Green tick only means review requested 😞 ).

finneyj · 2019-03-01T13:51:13Z

source/core/MicroBitFiber.cpp

+        queue = queue->next;
+    }
+}
+


Looks fine, but just checking that sum is intentionally not initialized to zero in this function? (I'm guess this is intentional from the variable names).

Can we get a doc comment block on this function?

finneyj · 2019-03-01T13:53:06Z

source/core/MicroBitFiber.cpp

+    // so it may in fact have the user_data set if in FOB context
+    if (dest)
+        *dest++ = idleFiber;
+    sum++;


Should this increment be inside the conditional? Looks like maybe it should?

No, the idea is that you call this twice, once with dest==NULL to count, then you allocate, and then call it again to get the data.

finneyj · 2019-03-01T13:56:32Z

source/core/MicroBitFiber.cpp

+    __disable_irq();
+    get_fibers_from(&dest, &sum, runQueue);
+    get_fibers_from(&dest, &sum, sleepQueue);
+    get_fibers_from(&dest, &sum, waitQueue);


Looks ok, but don't forget we introduced a mutex semaphore as part of js-event-semantics... Each mutex has its own wait queue associated with it, so you could perhaps miss extracting those fiber context with this function? Would that cause a problem for your application of this?

https://github.com/lancaster-university/microbit-dal/blob/js-event-semantics/source/core/MicroBitFiber.cpp#L944

finneyj · 2019-03-01T13:57:45Z

source/core/MicroBitFiber.cpp

+        }
+    }
+    return f;
+}


Great - I've been wanting to factor this out into a function for a while... Thanks @mmoskal!

finneyj · 2019-03-01T14:01:32Z

source/core/MicroBitFiber.cpp

+    if (f != currentFiber) {
+        dequeue_fiber(f);
+        queue_fiber(f, &runQueue);
+        schedule();


Hmmm.... I'm not sure this is entirely safe for this particular funciton. fiber_wake_on_event() is used to place the current fiber on a wait queue without entering the scheduler (such that race conditions can be avoided). i.e.

fiber_wake_on_event(..)
// do something that will cause that event to happen at some later point
schedule();

or am I missing something?

oh wait, the schedule() is there already on master; I don't quite understand why, but I think it's fine to align with codal-core

finneyj · 2019-03-01T14:05:26Z

source/core/MicroBitFiber.cpp

@@ -475,7 +523,7 @@ int invoke(void (*entry_fn)(void))
    if (!fiber_scheduler_running())
 		return MICROBIT_NOT_SUPPORTED;

-    if (currentFiber->flags & MICROBIT_FIBER_FLAG_FOB)
+    if (currentFiber->flags & (MICROBIT_FIBER_FLAG_FOB | MICROBIT_FIBER_FLAG_PARENT | MICROBIT_FIBER_FLAG_CHILD) || HAS_THREAD_USER_DATA)


@mmoskal Can you please describe the conditions when HAS_THREAD_USER_DATA is set? Will this affect all MakeCode application fibers / event handlers? Wouldn't this affect RAM efficiency and context switch time somewhat?

I also wonder if a global CONFIG option to disable FOB might be simpler if that's the case? Just thinking of future maintenance here. Would be good to get your thoughts.

finneyj · 2019-03-01T14:06:20Z

source/core/MicroBitFiber.cpp

+    // limit the number of fibers in the pool
+    int numFree = 0;
+    for (Fiber *p = fiberPool; p; p = p->next) {
+        if (!p->next && numFree > 3) {


OK, but can we bring this constant out as a CONFIG option please?

finneyj · 2019-03-01T14:08:08Z

source/core/MicroBitFiber.cpp

@@ -754,6 +828,8 @@ void verify_stack_size(Fiber *f)

        // Recalculate where the top of the stack is and we're done.
        f->stack_top = f->stack_bottom + bufferSize;
+
+        currentFiber = prevCurrFiber;
    }


is this just for the case where the code may be pre-empted by an interrupt that scans the fiber list? if so, it's a short piece of code, can we protect with a more traditional disable/enable IRQ?

finneyj · 2019-03-01T14:09:19Z

source/core/MicroBitHeapAllocator.cpp

+        // and remove the memset
+        ((uint32_t*)mem)[0] = 1;
+        memset(mem, 0, num*size);
+    }



cor - good spot.

finneyj · 2019-03-01T14:11:48Z

Thanks for all the work on this @mmoskal.

Generally all looks positive. Happy to merge pending questions I've posted as line comments - so could you take a look at those?

- Add new CONFIG option to enable per-fiber meta data (default: disabled) - Add (void *) pointer to Fiber struct when CONFIG option enabled - Add Yotta CONFIG glue - Refactor fork-on-block handling into a unified function - Align validation semantics of both flavours of invoke() function

mmoskal added 4 commits January 20, 2019 17:58

Add Fiber->user_data and list_fiber()

5300724

This is port of relevant codal changes in support of GC in the PXT runtime. Also, limit the number of fibers in pool to 3.

Allow overriding of the heap allocator and getting heap sizes

6275437

Also port from codal-core in support of PXT GC. Also, fix compiler optimization bug in calloc().

Compilation fixes

517f2ea

Add fiber_user_data to yotta mappings; fix some errors

6e9b1d5

mmoskal requested review from jamesadevine and finneyj February 22, 2019 00:00

mmoskal changed the title ~~port codal fiber changes from codal for PXT GC support~~ port fiber changes from codal for PXT GC support Feb 22, 2019

finneyj reviewed Mar 1, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port fiber changes from codal for PXT GC support #424

port fiber changes from codal for PXT GC support #424

mmoskal commented Feb 22, 2019

abchatra commented Feb 27, 2019

finneyj commented Feb 27, 2019

abchatra commented Feb 27, 2019

finneyj Mar 1, 2019

finneyj Mar 1, 2019

mmoskal Mar 1, 2019

finneyj Mar 1, 2019

finneyj Mar 1, 2019

finneyj Mar 1, 2019

finneyj Mar 1, 2019

mmoskal Mar 24, 2020

finneyj Mar 1, 2019

finneyj Mar 1, 2019

finneyj Mar 1, 2019

finneyj Mar 1, 2019

finneyj commented Mar 1, 2019

port fiber changes from codal for PXT GC support #424

Are you sure you want to change the base?

port fiber changes from codal for PXT GC support #424

Conversation

mmoskal commented Feb 22, 2019

abchatra commented Feb 27, 2019

finneyj commented Feb 27, 2019

abchatra commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finneyj commented Mar 1, 2019