Applab: fix dontMarshalApi functions for export

[cpirich]

Resolves an issue covered here: https://codedotorg.atlassian.net/browse/STAR-336

All of the APIs within dontMarshalApi.js have specific knowledge of interpreter data structures. In order to use these outside of the interpreter, we need to have equivalent versions that can run in a normal JavaScript environment.

This PR addresses that. Rather than introduce two versions of each function that will need to be maintained in parallel, I've decided to set things up so that a single implementation is available that can do some work conditionally when it is running within the interpreter (through a new calledWithinInterpreter argument)

Also, removed unused list functions from api.js - the real versions of these are in dontMarshalApi.js - we were exporting both versions and the first versions didn't do anything.

Added a basic correctness test for the list blocks (insertItem, appendItem, and removeItem) since I couldn't find any test coverage for these at all.

[jmkulwik]

There's a lot of duplicated code here. I'd like to try to find a way to remove some of the duplication. @joshlory and I were talking about this a bit offline, and we came up with a couple ideas.

1. Create a wrapper that passes input.properties if called within the interpreter. i.e.

wrap = (fn) => {
  return (...args) => {
    args.forEach((arg, n) =>
      if (arg.properties) {
        args[n] = arg.properties
      }
    );
    return fn.apply(this, args);
  }
}

2. Handle this further upstream so we pass the correct properties to functions like getRed and dontMarshalApi doesn't need to know the difference.
3. Refactor within JS-Interpreter to use a version that works for both native and JS-Interpreter (this is more for the array manipulation functions)

Do you know why we need to use arg.properties in so many places here?

[cpirich]

@jmkulwik @joshlory Can you be clear on which duplication you'd like to avoid? It is possible to make all of the getRed() and setRed() type functions wrap a simpler template, but I'm not sure that's what you're going for.

In response to your ideas:

(1) - doesn't handle any of the complex scenarios (like nested objects or arrays) or the need to create primitives in the response (things that are normally taken care of by the marshaling code)
(2) - this is essentially what marshaling does. we do that for something like 98% of our APIs. These are the exceptions because they either modify parameters (in/out params) [list functions], need high perf without marshaling huge arrays [get imagedata APIs], or both [set imagedata APIs]
(3) - for the list functions, I have some question as to why they were built this way. I was interested in making them simple wrappers of existing array functions (e.g. these could all be calls to Array.splice() which already has 2 implementations - one for interpreter, one built-in for normal JS). The only reason I didn't do this was because we've added strange behaviors (e.g. rounding the index param in case it is floating point?)

I can look at (3), but for now, but the fixes above are legit and the code as-is is completely broken. I'd be happy to get fixes merged in and then discuss improved fixes that are cleaner. Believe me, I thought about it for a little while. It's not a super simple problem.

[jmkulwik]

Comments left by @joshlory in Slack while Github was down:

It's annoying that we created "don't marshal" APIs in the first place.

Is it written down anywhere why we created this in the first place? (I think perf?) Looking back now, how do you think we should've done it instead?

Would it be possible to move insertItem, appendItem and removeItem out of dontMarshalApi.js and into the normal API?

[joshlory]

To expand: what's the difference between "custom marshal" and "don't marshal". Can we update marshaling in general to treat these data types as special, so we don't need to special-case the APIs that pass them as arguments or return values?

[cpirich]

We can't move them out of dontMarshalApi.js because they modify the arrays that are passed as a parameter to them. Our default marshaling is "by value". Arguments/parameters are "copied" on the way in, and the return value is "copied" on the way out.

"custom marshal" allows us to essentially keep objects in the interpreter as thin wrappers around data that is actually maintained outside the interpreter. As an example, a Sprite in gamelab is always custom marshaled. When it is passed as a parameter, we don't copy it (by value) from "native" to "interpreted", we create a interpreter wrapper (essentially passing it by reference).

"don't marshal" is used in applab for higher-performance scenarios (ImageData) or where we want to modify a parameter (since marshaling copies, changes made in the copy within the native function have no impact in the interpreted world)

[jmkulwik]

I'm ok with this as-is to fix the current issue, but only if it comes with a follow-up task this or next sprint to address the duplicated code and add testing for the native and interpreted code paths. I know @joshlory had some comments that he left in slack since github was down. I'll wait for him to sign-off for this change as these haven't been addressed yet. I'll copy those comments over to the CR thread here.

[cpirich]

@jmkulwik @joshlory
I committed an additional change to reduce some duplication:

1. List function (insertItem, appendItem, removeItem) parameter validation no longer requires conditionals (validateType validator is passed to the inner function, function signatures are now identical between the two types of parameter validation functions)
2. All ImageData functions (e.g. getRed, setRed) except setRGB are now implemented on top of shared inner functions called getImageDataValue and setImageDataValue

Verified these changes inside and outside the interpreter.

Can one of you take another look and approve if you're ok with getting this fix in?

[joshlory]

Verified these changes inside and outside the interpreter.

It looks like there's a test for the behavior inside the JS-Interpreter. Can we get a test for calling the function outside (like in the the export case) also?

[joshlory]

This may be unavoidable, but want to call out that we're explicitly depending on JS-Interpreter internal state (how it stores underlying data).

[cpirich]

absolutely - anything operating at this level (effectively trying to augment interpreter functionality) is subject to this. It may be avoidable in the long term if we want to create more of a formal interface around how to reach in to interpreter specific data.

[cpirich]

Verified these changes inside and outside the interpreter.

It looks like there's a test for the behavior inside the JS-Interpreter. Can we get a test for calling the function outside (like in the the export case) also?

Yes, I just added a bunch of unit tests for the native JS list functions.

[joshlory]

The tests for the dontMarshalApi.js methods look great, but I'm a little worried we haven't tested the original bug that prompted the fix end-to-end. I'm going to add a test case to ExporterTest.js if that's ok!

[jmkulwik]

Thank you for making this fix and being active in all the discussions around the js interpreter!

[cpirich]

The tests for the dontMarshalApi.js methods look great, but I'm a little worried we haven't tested the original bug that prompted the fix end-to-end. I'm going to add a test case to ExporterTest.js if that's ok!

Perfect, thank you!