[Batch - PART2] Batch operations and custom data #80

Mobius5150 · 2018-06-09T01:18:32Z

Stacked PR on #81

Exposing a new batch operations API that allows you to batch updates to
multiple objects (e.g. controls, participants) and multiple property
updates on a single object. This API also allows for the use of custom
data including nested objects and arrays.

Sample setting custom data on a control object:

interactive_batch batch;
interactive_control_batch_begin(session, &batch, "default");

interactive_batch_entry entry;
interactive_control_batch_add(batch, &entry, "GiveHealth");
interactive_batch_add_param_str(batch, entry, "foo", "bar");
interactive_batch_add_param_uint(batch, entry, "number", 42);
interactive_batch_add_param_array(batch, entry, "array", [](interactive_batch batch, interactive_batch_array arrayItem)
{
    interactive_batch_array_push_object(batch, arrayItem, [](interactive_batch batch, interactive_batch_entry objectEntry)
    {
        interactive_batch_add_param_str(batch, objectEntry, "foo", "bar");
        interactive_batch_add_param_uint(batch, objectEntry, "number", 42);
    });
    interactive_batch_array_push_str(batch, arrayItem, "bar");
    interactive_batch_array_push_uint(batch, arrayItem, 42);
});
interactive_batch_add_param_object(batch, entry, "object", [](interactive_batch batch, interactive_batch_entry objectEntry)
{
    interactive_batch_add_param_str(batch, objectEntry, "foo", "bar");
    interactive_batch_add_param_uint(batch, objectEntry, "number", 42);
});
interactive_control_batch_end(batch);

Results in:

{
   "id":4,
   "method":"updateControls",
   "discard":true,
   "seq":5,
   "params":{
      "priority":0,
      "controls":[
         {
            "controlID":"GiveHealth",
            "foo":"bar",
            "number":42,
            "array":[
               {
                  "foo":"bar",
                  "number":42
               },
               "bar",
               42
            ],
            "object":{
               "foo":"bar",
               "number":42
            }
         }
      ],
      "sceneID":"default"
   }
}

Batch operations supported for:

Tests are not yet complete as we have yet to expose handlers for control
update events and the current implementation does not update internal
state on all events so it is not testable. We also need to invest in
proper teardown methods to kill interactive connection when using
asserts.

DO NOT MERGE

Exposing a new batch operations API that allows you to batch updates to multiple objects (e.g. controls, participants) and multiple property updates on a single object. This API also allows for the use of custom data including nested objects and arrays. Current only exposes batch methods for `controls` - other objects will come in a subsequent commit. Tests are not yet complete as we have yet to expose handlers for control update events and the current implementation does not update internal state on all events so it is not testable. We also need to invest in proper teardown methods to kill interactive connection when using asserts.

Fix build issues and warnings and have working tests for control updates

JoshSnider · 2018-06-18T22:13:48Z

source/interactivity.h

+	/// <summary>
+	/// Opaque handle for a batch update object
+	/// </summary>
+	struct interactive_batch_public {};


Since everything in interactivity.h is public can we leave off the _public suffix? The existing paradigm is to mark internal structs with an _internal suffix to differentiate.

Also, what do you think about changing just this structure name to interactive_batch_op? A little more self documenting as it sort of indicates what you would do with the object. (begin, commit)

That depends - there's already a typedef below for interactive_batch which is the type that external developers will use. Adding _op_ is cool, but let me know what you want to do re _public. This struct is something they'll likely not reference - it's just there for type safety

Gotcha, sorry misread this the first time. _public is fine.

JoshSnider · 2018-06-18T22:21:59Z

source/interactivity.h

+
+	typedef interactive_batch_array_public* interactive_batch_array;
+
+	typedef void(*interactive_batch_object_callback)(interactive_batch, interactive_batch_entry);


interactive_batch_entry_callback?

JoshSnider · 2018-06-18T22:26:02Z

source/interactivity.h

+	/// <para>The Mixer interactive protocol implements the JSON Merge Patch algorithm (RFC 7386) for updates on protocol objects - meaning that to delete an object property you need to set it to null rather than just omit it in an update.</para>
+	/// </remarks>
+	/// </summary>
+	int interactive_batch_add_param_null(interactive_batch batch, interactive_batch_entry entry, const char* name);


Can we simplify this to just one handle (interactive_batch_entry entry, const char* name)? Ideally the batch and the entry are decoupled except for when they are linked with the interactive_*_batch_add functions.

Sure, means another ptr in the struct but not the end of the world

Yeah the struct gets a little larger but makes the calling pattern much cleaner in that you only have to pass the handle to the function that deals with that handle (begin/commit for batch_op, add_param for entry).

JoshSnider · 2018-06-18T22:28:59Z

source/interactivity.h

+	/// <para>Note that a batch request should only contain a single entry per participant ID.</para>
+	/// </remarks>
+	/// </summary>
+	int interactive_participant_batch_add(interactive_batch batch, interactive_batch_entry* entry, const char* participantId);


What error do I get if I do this?

interactive_batch batch; interactive_participant_batch_begin(session, &batch); interactive_entry entry; int err = interactive_control_batch_add(batch, &entry, "GiveHealth");

Good catch - missed that. But let's decide on a direction for #80 (comment) before I fix

JoshSnider · 2018-06-18T22:30:08Z

source/interactivity.h

+	/// <summary>
+	/// Adds an update to the participant batch request for the participant with the given id.
+	/// <remarks>
+	/// <para>Note that a batch request should only contain a single entry per participant ID.</para>


Is this required or just a suggestion?

Required right now. We could workaround with a decent amount of work. Would increase the footprint of an entry object by 2 * size_t for each protocol object: world, group, scene, participant, control.

Doing this could simplify calling patterns too. Thoughts?

Actually... Nevermind the above comment. This wouldn't be a good idea.

Right now the protocol only supports updates for a single type of object in a single method call. We could abstract this away BUT it would mean that locally we would turn this into as many participantUpdate, controlUpdate, etc... calls as were needed. But the problem is that the calling pattern suggests the update would be performed as a single atomic operation when it would actually be performed as many independent operations, potentially out of order, each of which could fail.

So let's not do that and call this a requirement for now.

Yeah totally agreed. I meant if you add an entry for an already existing ID does that break? Can we enforce it with an error message or just clean it up and do a merge?

Would love to see test cases that also call these in odd ways and assert errors.

JoshSnider · 2018-06-18T22:34:30Z

source/internal/interactive_batch.h

+struct interactive_batch_internal
+{
+	interactive_session_internal* session;
+	const char* method;


Who owns this and param below?

The respective batch begin method, e.g. https://github.com/mixer/interactive-cpp/pull/80/files/2337d46dacb92bc2621421564b7f6a5988c80a61#diff-426a8659af90d0fe57bec68619f83941R20

See comment below about modifying the document directly rather than needing to keep param.

Both strings need to be valid until _commit is called which is why they aren't copied.

That will probably manifest in runtime errors where the caller cleans up the string before calling commit. Can we do a copy here and use std::string? It's a known runtime impact (2 string copies per batch) and shouldn't be a perf issue.

JoshSnider · 2018-06-18T23:00:34Z

source/internal/interactive_session.cpp

@@ -10,8 +10,11 @@ typedef std::function<void(rapidjson::Document::AllocatorType& allocator, rapidj

 int create_method_json(interactive_session_internal& session, const std::string& method, on_get_params getParams, bool discard, unsigned int* id, std::shared_ptr<rapidjson::Document>& methodDoc)
 {
-	std::shared_ptr<rapidjson::Document> doc(std::make_shared<rapidjson::Document>());
-	doc->SetObject();
+	std::shared_ptr<rapidjson::Document> doc(nullptr == methodDoc ? std::make_shared<rapidjson::Document>() : methodDoc);


The reason this was not written this way initially was to preserve output parameter state until we are ready to return. If an error state occurs you would not want to have modified methodDoc. This seems okay since there are no error states below but would have to be rewritten if there were (unlikely). However I'm more interested in the reason for the change, is there a need to re-use the incoming pointer? It would seem that you would always expect a clean json object as well but if you pass in a non-null object this would add to it, somewhat violating the 'create_method_json' name for the function.

Good points. Two reasons:

Efficiency on the calling thread (already have the document object on hand)

I was working towards having the batch begin function be able to customize the document before getParams is called so that we don't have to keep state in the batch entry in the future.

Also is there a better way to get the rapidjson allocator than creating a document? Would there ever be concern with creating the json objects with an allocator from document A then using in document B?

Could always split this call into another function.

The allocator is bound to the document, definitely can't mix and match them. rapidjson has functions to move objects around without copies which might solve this.

JoshSnider · 2018-06-18T23:32:33Z

source/internal/interactive_batch.cpp

+		freePtr = next;
+		next = next->next;
+		delete freePtr;
+	}


If there's a failure between interactive_batch_begin and interactive_batch_commit, will all created entries leak?

Good point. I'll make interactive_batch_free public so you can call it in an error state. Would then also want to make _commit not free the object so you consistently call _free every time.

That said, what do you think about having interactive_batch_commit free the entries (so you don't iterate the linked list again) but not the batch op object? This detail shouldn't be exposed to the user at all.

I think we can get away with not having the caller own anything but interactive_batch_op which would greatly simplify memory management. See my comment below and let's chat.

JoshSnider · 2018-06-19T17:46:30Z

source/internal/interactive_batch.cpp

+{
+}
+
+int interactive_batch_begin(interactive_session session, interactive_batch* batchPtr, char *methodName, on_get_batch_params getParams)


By convention output parameters are always the last parameters so this should be (interactive_session session, char* methodName, on_get_batch_params getParams, interactive_batch* batchPtr)

JoshSnider · 2018-06-19T20:36:33Z

What do you think about this?

interactive_batch_op batchOp;
interactive_batch_begin(session, "default", interactive_batch_type_control, interactive_batch_op_type_update, &batchOp);

interactive_batch_entry;
interactive_batch_add(batchOp, "GiveHealth", &entry);

interactive_batch_add_param_str(entry, "foo", "bar");
interactive_batch_add_param_uint(entry, "number", 42);

interactive_batch_array arrayItem;
interactive_batch_add_param_array(entry, "array", 3, &arrayItem);

interactive_batch_entry arrObject;
interactive_batch_array_push_object(arrayItem, "object", &arrObject);
interactive_batch_add_param_str(arrObject, "foo", "bar");
interactive_batch_add_param_uint(arrObject, "number", 42);    

interactive_batch_array_push_str(arrayItem, "array", "bar");
interactive_batch_array_set_uint(arrayItem, 2, 42); // Set index '2' to 42

interactive_batch_entry objectEntry;
interactive_batch_add_param_object(entry, "object", &objectEntry);
interactive_batch_add_param_str(objectEntry, "foo", "bar");
interactive_batch_add_param_uint(objectEntry, "number", 42);

interactive_batch_commit(batchOp);
interactive_batch_close(batchOp);

Mobius5150 · 2018-07-14T23:23:05Z

@JoshSnider Let me know if we're looking good now and I'll do the rest of the object types

Mobius5150 requested a review from JoshSnider June 9, 2018 01:18

Mobius5150 changed the title ~~WIP: feat(batch): Initial commit for batch operations~~ WIP: feat(batch): Batch operations and custom data Jun 9, 2018

Mobius5150 added 2 commits June 15, 2018 13:15

feat(batch): Control tests and build fixes

a890253

Fix build issues and warnings and have working tests for control updates

Mobius5150 force-pushed the feat/batch-operations branch from b233445 to a890253 Compare June 15, 2018 20:56

Mobius5150 changed the base branch from master to feat/control-updates June 15, 2018 20:56

Mobius5150 changed the title ~~WIP: feat(batch): Batch operations and custom data~~ [Batch - PART2] Batch operations and custom data Jun 15, 2018

feat(batch): Comment batch method declarations

2337d46

JoshSnider reviewed Jun 18, 2018

View reviewed changes

JoshSnider reviewed Jun 19, 2018

View reviewed changes

Mobius5150 added 4 commits July 14, 2018 15:39

feat(batch): Batch implementation w/o callback

dd2cec3

fix(feedback): Apply PR feedback

3f734bd

feat(batch): Improve tests for control batch ops

1d2c928

fix(batch): PR feedback on naming

f2de220

feat(bash): Refactor macro

70e437b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Batch - PART2] Batch operations and custom data #80

[Batch - PART2] Batch operations and custom data #80

Mobius5150 commented Jun 9, 2018 •

edited by JoshSnider

Loading

JoshSnider Jun 18, 2018

JoshSnider Jun 18, 2018

Mobius5150 Jun 19, 2018

JoshSnider Jun 19, 2018 •

edited

Loading

JoshSnider Jun 18, 2018

Mobius5150 Jun 19, 2018

JoshSnider Jun 18, 2018

Mobius5150 Jun 19, 2018

JoshSnider Jun 19, 2018

JoshSnider Jun 18, 2018

Mobius5150 Jun 19, 2018

JoshSnider Jun 18, 2018

Mobius5150 Jun 19, 2018

Mobius5150 Jun 19, 2018

JoshSnider Jun 19, 2018

JoshSnider Jun 18, 2018

Mobius5150 Jun 19, 2018

JoshSnider Jun 19, 2018 •

edited

Loading

JoshSnider Jun 18, 2018

Mobius5150 Jun 19, 2018

JoshSnider Jun 19, 2018

JoshSnider Jun 18, 2018

Mobius5150 Jun 19, 2018

JoshSnider Jun 19, 2018

JoshSnider Jun 19, 2018

JoshSnider commented Jun 19, 2018

Mobius5150 commented Jul 14, 2018


		typedef interactive_batch_array_public* interactive_batch_array;

		typedef void(*interactive_batch_object_callback)(interactive_batch, interactive_batch_entry);

[Batch - PART2] Batch operations and custom data #80

Are you sure you want to change the base?

[Batch - PART2] Batch operations and custom data #80

Conversation

Mobius5150 commented Jun 9, 2018 • edited by JoshSnider Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshSnider Jun 19, 2018 • edited Loading

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

JoshSnider Jun 19, 2018 • edited Loading

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

JoshSnider commented Jun 19, 2018

Mobius5150 commented Jul 14, 2018

Mobius5150 commented Jun 9, 2018 •

edited by JoshSnider

Loading

JoshSnider Jun 19, 2018 •

edited

Loading

JoshSnider Jun 19, 2018 •

edited

Loading