Spawn behaviours with a dynamic cown set #23

marioskogias · 2023-05-25T21:23:18Z

No description provided.

mjp41

LGTM. Could you improve the test coverage?

src/rt/cpp/when.h

mjp41 · 2023-08-21T08:43:58Z

test/func/dynamic-cownset/dynamic-cownset.cc

+  auto log3 = make_cown<Body2>(1);
+
+  cown_ptr_span<Body2> t2{&log3, 1};


Is this dead code?

mjp41 · 2023-08-21T08:44:51Z

test/func/dynamic-cownset/dynamic-cownset.cc

+  auto log3 = make_cown<Body2>(1);
+
+  cown_ptr_span<Body2> t2{&log3, 1};


Is this dead code?

test/func/dynamic-cownset/dynamic-cownset.cc

mjp41 · 2023-08-21T08:48:07Z

test/func/dynamic-cownset/dynamic-cownset.cc

+
+  cown_ptr_span<Body2> t2{&log3, 1};
+
+  when(t1, log1) << [=](acquired_cown_span<Body1> ca, acquired_cown<Body1> a) {


Could you add a few more complex sequences here to ensure coverage of first and last being different things:

span, cown

cown, span

span, span

span, cown, span

cown, span, cown

test/func/dynamic-cownset/dynamic-cownset.cc

mjp41 · 2023-08-21T08:52:36Z

test/func/dynamic-cownset/dynamic-cownset.cc

@@ -0,0 +1,102 @@
+// Copyright Microsoft and Project Verona Contributors.


Could you add test coverage for move versus copy of cown span

when (span) { ... }

versus

when (std::move(span)) { ... }

mjp41 · 2023-08-21T16:41:09Z

src/rt/cpp/when.h

+      size_t actual_size =
+        ptr_span.length * sizeof(ActualCown<std::remove_const_t<T>>*);
+      size_t acq_size =
+        ptr_span.length * sizeof(acquired_cown<std::remove_const_t<T>>);
+      span.array = reinterpret_cast<ActualCown<std::remove_const_t<T>>**>(
+        snmalloc::ThreadAlloc::get().alloc(actual_size + acq_size));
+
+      for (size_t i = 0; i < ptr_span.length; i++)
+      {
+        span.array[i] = ptr_span.array[i].allocated_cown;
+      }
+      span.length = ptr_span.length;
+
+      acq_array =
+        reinterpret_cast<acquired_cown<T>*>((char*)(span.array) + actual_size);
+
+      for (size_t i = 0; i < ptr_span.length; i++)
+      {
+        new (&acq_array[i]) acquired_cown<T>(*ptr_span.array[i].allocated_cown);
+      }


Could you factor out the code duplication here from the above constructor. Thanks

mjp41

Mostly minute stuff now. Would be nice if there were a few more comments. But it's looking great.

src/rt/cpp/when.h

mjp41 · 2023-08-22T05:39:22Z

src/rt/cpp/when.h

+    cown_ptr_span(cown_ptr_span&& old) = delete;
+    cown_ptr_span& operator=(cown_ptr_span&&) = delete;
+    cown_ptr_span& operator=(const cown_ptr_span&) = delete;
+  };


Why are these operators deleted? Could you add a comment to explain.

There's no need to be deleted. They are not used in the current codebase. So, I marked them as deleted to avoid bugs in auto generated. I'll add a comment to clarify.

mjp41 · 2023-08-22T05:40:48Z

src/rt/cpp/when.h

+    {
+      array = reinterpret_cast<cown_ptr<T>*>(
+        snmalloc::ThreadAlloc::get().alloc(length * sizeof(cown_ptr<T>*)));
+      memset(array, 0, length * sizeof(cown_ptr<T>*));


Why memset? The next loop overwrites everything.

Because the cown_ptr copy assignment operator does a clear() first. I'll remove the memset and call the copy constructor with new (&array[i]) cown_ptr<T>(arr[i]) instead.

src/rt/cpp/when.h

mjp41 · 2023-08-22T05:52:38Z

src/rt/cpp/when.h

+  template<typename T>
+  struct cown_ptr_span
+  {
+    cown_ptr<T>* array;


Could you add comments about ownership here? Is this class opening the underlying cownptrs or borrowing them. Explaining that, and then adding comments elsewhere.

Does std::move make sense if they are borrowed? Hadn't thought about this for my earlier review.

I think it's clearer if cown_ptr_span always owns the included cowns. This can happen either by allocating a new cown_ptr array and incrementing the reference count, or by using the template parameter and passing an array of cown_ptr that already holds the ownership.

I added a comment that explains that.

mjp41 · 2023-08-23T19:43:36Z

src/rt/cpp/cown_span.h

+   * allocated array.
+   */
+  template<typename T>
+  struct cown_ptr_span


Sorry, I have one minor thing given this is an owning array. I think we should perhaps call it: cown_array. I think of span as borrowed, and not owning the underlying memory. But this does, so I think the current name will given the wrong idea.

Sure I can do that. I thought that owning made more sense and made things simpler. Alternatively, we can keep the span semantics and prevent from passing it with move. What do you think?

I think owning is the right choice. Let's rename.

src/rt/cpp/when.h

mjp41 · 2023-08-24T09:05:49Z

src/rt/cpp/when.h

+    ActualCownSpan& operator=(ActualCownSpan&& old)
+    {
+      if (array)
+        snmalloc::ThreadAlloc::get().dealloc(array);


This is performing deallocation, so doesn't this require ownership?

This array holds ActualCown<T>, though, not cown_ptr<T>. If I understand correctly, when this array is constructed there is no reference count increase.

Okay, yeah, so ActualCown is kind of borrowing from the underlying cown_ptr. Makes sense, do you think you could add a comment, so we don't get confused again. Possibly add a comment to ActualCown too?

Should there be a destructor to deallocate the array too?

You are right. ActualCown borrows from the underlying cown_array. I'll add the comment.

You are also correct about the confusion with the dealloc. This is wrong and it shouldn't be there. The lifetime of the ActualCownSpan is always the same as the AccessBatch. The memory the span uses is managed by the AccessBatch constructors and destructors. There is no need for a move assignment operator. I'll remove that.

The lifetime of the Access and AccessBatch goes all the way till after the execution of the behaviour.

Given that this the case, should the ActualCownSpan be defined inside the AccessBatch?

Yeah. So ActualCownSpan is different to ActualCown. ActualCown is the Verona runtimes view of a cown, where as ActualCownSpan is part of the behaviour structure. I think moving it into AccessBatch makes sense, I also wonder if it should be Actual..., but differently named?

Actually, it should be ActualCownPtrSpan since it's an array of pointers to ActualCown.

Ignore it. It's not necessary and it's only confusing. I'm removing it completely.

mjp41

LGTM, could you remove the dead code.

mjp41 · 2023-08-24T14:56:23Z

src/rt/cpp/when.h

+#if 0
+    /**
+     * This is a non-owning span of ActualCown pointers
+     * Its lifetime is always the same as the AccessBatch.
+     */
+    struct ActualCownPtrSpan
+    {
+      ActualCown<std::remove_const_t<T>>** array;
+      size_t length;
+
+      void clear()
+      {
+        length = 0;
+        array = nullptr;
+      }
+
+      ActualCownPtrSpan()
+      {
+        clear();
+      }
+
+      ActualCownPtrSpan(ActualCownPtrSpan&& old)
+      {
+        length = old.length;
+        array = old.array;
+        old.clear();
+      }
+    };
+#endif
+


Suggested change

#if 0

/**

* This is a non-owning span of ActualCown pointers

* Its lifetime is always the same as the AccessBatch.

*/

struct ActualCownPtrSpan

{

ActualCown<std::remove_const_t<T>>** array;

size_t length;

void clear()

{

length = 0;

array = nullptr;

}

ActualCownPtrSpan()

{

clear();

}

ActualCownPtrSpan(ActualCownPtrSpan&& old)

{

length = old.length;

array = old.array;

old.clear();

}

};

#endif

mjp41

LGTM

marioskogias requested a review from mjp41 May 25, 2023 22:34

marioskogias force-pushed the batch-sched branch from 9a2f052 to 2e0cfbd Compare May 26, 2023 17:17

marioskogias added 6 commits August 9, 2023 19:09

Copy the old bench

9a9790a

Compiling Access free

99cfac8

Progress

d9acb6f

Allocate extra space for batch

1644904

Populate acquired_cown_span

1d28671

Get something to compile

071a4c4

marioskogias force-pushed the batch-sched branch from be94f60 to 071a4c4 Compare August 10, 2023 11:04

marioskogias added 3 commits August 10, 2023 13:35

Add tests

842f08c

Fix bugs

8816f0f

Attempt windows compilation fix

42cd608

marioskogias force-pushed the batch-sched branch from bc886c2 to 42cd608 Compare August 10, 2023 17:04

mjp41 requested changes Aug 21, 2023

View reviewed changes

Address feedback and add tests

194234d

mjp41 reviewed Aug 21, 2023

View reviewed changes

marioskogias added 2 commits August 21, 2023 19:18

Fix failing tests

5e503ca

Fix windows compilation

23259c5

marioskogias force-pushed the batch-sched branch from 64c9b98 to 23259c5 Compare August 21, 2023 18:38

mjp41 requested changes Aug 22, 2023

View reviewed changes

Address more feedback

4c66aea

marioskogias force-pushed the batch-sched branch from 43ffdef to 4c66aea Compare August 22, 2023 09:06

mjp41 reviewed Aug 23, 2023

View reviewed changes

Rename

f4de5fe

mjp41 reviewed Aug 24, 2023

View reviewed changes

src/rt/cpp/when.h Outdated Show resolved Hide resolved

mjp41 reviewed Aug 24, 2023

View reviewed changes

Clarify ownership

2fb7814

mjp41 approved these changes Aug 24, 2023

View reviewed changes

Remove ActualCownSpan

2bf3458

marioskogias force-pushed the batch-sched branch from 10debf8 to 2bf3458 Compare August 24, 2023 15:34

mjp41 approved these changes Aug 24, 2023

View reviewed changes

marioskogias merged commit ab42a9f into microsoft:main Aug 24, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spawn behaviours with a dynamic cown set #23

Spawn behaviours with a dynamic cown set #23

marioskogias commented May 25, 2023

mjp41 left a comment

mjp41 Aug 21, 2023

mjp41 Aug 21, 2023

mjp41 Aug 21, 2023

mjp41 Aug 21, 2023

mjp41 Aug 21, 2023

mjp41 left a comment

mjp41 Aug 22, 2023

marioskogias Aug 22, 2023

mjp41 Aug 22, 2023

marioskogias Aug 22, 2023

mjp41 Aug 22, 2023

marioskogias Aug 22, 2023

mjp41 Aug 23, 2023

marioskogias Aug 23, 2023

mjp41 Aug 24, 2023

marioskogias Aug 24, 2023

mjp41 Aug 24, 2023

marioskogias Aug 24, 2023

mjp41 Aug 24, 2023

mjp41 Aug 24, 2023

marioskogias Aug 24, 2023

marioskogias Aug 24, 2023

mjp41 Aug 24, 2023

marioskogias Aug 24, 2023

marioskogias Aug 24, 2023

mjp41 left a comment

mjp41 Aug 24, 2023

mjp41 left a comment

		auto log3 = make_cown<Body2>(1);

		cown_ptr_span<Body2> t2{&log3, 1};


		cown_ptr_span<Body2> t2{&log3, 1};

		when(t1, log1) << [=](acquired_cown_span<Body1> ca, acquired_cown<Body1> a) {

		@@ -0,0 +1,102 @@
		// Copyright Microsoft and Project Verona Contributors.

Spawn behaviours with a dynamic cown set #23

Spawn behaviours with a dynamic cown set #23

Conversation

marioskogias commented May 25, 2023

mjp41 left a 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

mjp41 left a 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

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

mjp41 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjp41 left a comment

Choose a reason for hiding this comment