'Thanks for working on this' Alex, I'm just continuing what you started (in both cases - reflection in general and AMFBinaryData). Thanks for building the foundation!
I did have AMF support also in mind when I was originally working on fleshing out the reflection stuff, so I'm pleased to be able to try to close some of the gaps now. I think the AMF code resembles the original javascript lib that you ported a lot less now. As an aside, that original js code had some issues which I can only assume were matched in the serverside implementation that it was paired with, otherwise I would have expected those to have been addressed (things like some of the reference tracking not being aligned and a couple of issues with the way NaN was being encoded iirc, and others). Anyway I think with side by side Unit testing now it is much, much easier to work on than it was a few months back. 'maybe we can find a pattern where the initializers can be added or removed at publish time, so that we don't force a position on this topic in the framework.' For the js-default-initializers, I'm guessing you might be thinking something like inlining a comment that could be removed during file copy into the local project? Something like: /** * @private * @type {Array} */ MyInitialView.prototype.MyInitialView_warnings //js-default-intializers=false = null; And the copying task could do a string replace of ''//js-default-intializers=false" with "" if the local project overrides it? Something like that could be flexible, I guess, as long as it does not slow down the compiler task too much, so yes - it (or some other idea you might have perhaps) sounds good to me. Other points: My general question was more... are we sure it makes a meaningful difference to have this switched off for a 'normal' sized project. Is the main goal of not having initializers defined in the framework to make startup 'faster' or to reduce the minified output size (or both). As an aside, I read something (can't remember exactly where) recently suggesting that the approach we are currently using for the minification part with string aliasing (single short variables for common string literals) is not recommended. I did not dig deeper at this point to check that or the validity of it, but the argument against it was because when the javascript is gzipped it compresses better with the duplicated string literals as they were originally. And the code executes faster once it is expanded because each string literal is parsed directly instead of looked up. Like I said, I did not check this, but actually it made some sense to me, it was just not how I had been thinking about things... I was only thinking about the file size of the minified js, not what happens to it when it is deployed. In fact I had previously tried splitting all the qName values in the reflection data to see if they could be minimised further using the string aliasing by isolating common long package sequences. But GCC decides that is not worth doing and recombines them all into the original strings instead of storing their parts. So the reason I think it is doing that is probably what I just mentioned. I will check this in the coming week and report back. I mention this here only to illustrate that what seems like a logical assumption for 'performance' may not be unless it is viewed from different angles... in this case, some file size minification settings may result in a larger gzipped version of the file than if they were not applied (and also perhaps result in code that runs slightly slower). Likewise, not initializing by default may provide some faster startup, but could perhaps have implications for performance during use of the app (for slower lookups of the undefined properties). If that turned out to be true, which performance is the most important? (this is just hypothetical at this point, I did not try to test this yet). I guess having the option to switch would serve both approaches. In terms of 'conformance vs performance' I think that the less you adhere to a standard the more you end up with that same type of issues that we avoided the browser for and appreciated the flash avm so much for in the first place. Essentially we are trying to get back to that level of reliability and certainty and consistency. I have not tried XML or Proxy yet (apart from encountering it with RemoteObject I think). I'll look at these in the coming days. I would hope there could be ways to get more developer support for runtime checking things for untyped access by wrapping the heavy checking code with if (goog.DEBUG) {do checks and issue warnings etc} so it is only present in debug build? This way at least it will provide some good clues to developers when things are not working right. there could even be a define to keep them in release build if preferred (but off by default :) ) Just some thoughts (again I have not looked at the specific things you mentioned yet, maybe I will get a reality check when I do!) (I mentioned that I would start a thread today to update on AMFBinaryData and discuss some things there, but just to confirm, I will do that tomorrow now) On Sat, Mar 2, 2019 at 7:29 PM Alex Harui <aha...@adobe.com.invalid> wrote: > Greg, > > Thanks for working on this. Better Reflection in a PAYG manner and > IExternalizable support were much needed. > > I also recall that js-default-initializers was going to default to be on > and framework configs would turn it off. Feel free to make that change. > > I think in AS, you can't for..in enumerate methods and getter/setters, so > feel free to take a shot at turning off their enumeration in > Object.defineProperties and see if it bloats the code or not, or has other > side-effects. > > I think the only place we'll disagree is on conformance vs performance. I > said this before, maybe I'm too pessimistic, but I do not believe we can > efficiently make every line of AS that worked in Flash work in the > browser. Especially around un-typed access to Proxy and XML. And thus, > that's why I'm going to choose performance over conformance. You'll > probably have to tweak something in your source code to use Royale in a > browser. > > That said, it just occurred to me that maybe we can find a pattern where > the initializers can be added or removed at publish time, so that we don't > force a position on this topic in the framework. > > Thoughts? > -Alex > > On 3/1/19, 4:08 PM, "Greg Dove" <greg.d...@gmail.com> wrote: > > Just a few notes to share on reflection changes, and a couple of > optional > discussion points if anyone wants to expand on them... > [I will follow up tomorrow with notes on the AMFBinaryData changes] > > *amf_updates branch* has the following: > > -[asjs] *Fixes *to some breaking changes introduced to* reflection > classes*, > and *restores manualtests:UnitTests to working order* > -[compiler, asjs] > *Addition of isDynamic:true in ROYALE_CLASS_INFO*. Non-dynamic classes > do > not have this added. *Related utility functions are added in reflection > package*. > -[compiler, asjs] > *Addition of compileFlags uint value to ROYALE_REFLECTION_INFO and > integration with reflection package* (CompilationData class). > What/why: > This permits reflection to detect if certain compilation options were > used > or not when considering the data it finds about classes. This is > particularly important when inspecting dynamic instances, which in its > current form requires 'js-default-initializers' to be active for the > inheritance chain of the inspection target (it uses the defined > properties > on the protoytpe chain to collate 'known' properties, so it can > isolate the > 'unknown' dynamic properties on an instance). > This is needed because we don't have class members defined with > non-enumerable status. > There is a new CompilationData class in Reflection that allows to check > whether certain compilation options were used at runtime on an > inspection > target. This only makes sense for javascript target so far, I am not > sure > it will be ever needed for swf, but the same type of thing may be > useful > for other future targets. > > If js-default-initializers is true then a class compiled with that > option, > if it has some static fields, also means that its > ROYALE_REFLECTION_INFO > gets a startup collation of it's static fields, for similar runtime > reflection support. > > Both these approaches work (so far, in testing) in debug and release > modes, > but there could be some issues with non-royale base classes that add > properties to the current instance, and are not known in the prototype > chain, or when extending a framework class which was compiled without > js-default-initializers, even though the local project does use that > option. In this case, when using 'getDynamicFields' reflection > function, > the developer receives a runtime warning in debug build, but the code > that > generates the warning is not present in release build (if the > developer is > happy to ignore it). > > This functionality is demonstrated in the following tests: > > manualtests/UnitTests/src/main/royale/flexUnitTests/reflection/ReflectionTesterTestDynamic.as > > *see general examples of testing dynamic reflection here [1]* > *see CompilationData example here [2]* > > I've tried to work withiin the constraints of the current setup. I had > to > add some things to the output to get a basic level of support for > runtime > inspection of dynamic properties. But it is very minimal. > > -[compiler, asjs] > *Changes to variables data in reflection, and in reflection classes* > > Reflection variables data (public vars) now includes the ability to > request > a single dual purpose getter/setter function for each reflection > variable > target. This means that *reflection can now work correctly for both > publc > accessors and public variables in release mode*, because it will use > the > renamed property in the getttersetter function. The gettersetter > function > object is not instantiated unless it is requested from reflection > data, so > this is not adding a new default accessor to the class, just an > on-demand > reflection function which works with renamed variables. > > *VariableDefinition and AccessorDefinition now include getValue and > setValue accessors* which return a dynamically generated getter > function > and a setter function respectively. > > Whether these getter/setter functions require an instance argument as > the > first argument when they are called or not depends on whether the > reflected > member is a static or instance member. I've started to add debug-build > only > basic checks to parameters etc which are not present in release build. > > > Discussion points > js-default-initializers > This is currently off by default. I thought we had discussed at some > point > that it should be on by default. I recall something like this, with the > suggestion that it should always be off for the framework builds. I am > not > fully convinced with that last point, because although it may run > startup > slightly faster, I am not sure that it really saves bytes in a gzipped > javascript (at some point I will check this), and it may theoretically > slow > runtime performance if there are a lot of undefined fields which > require > full inspection of the prototype chain each time their value is > requested > (instead of finding the value defined explicitly along the chain and > 'returning' earlier - again, I did not check this yet either, but it > seems > logical to me). > This type of 'intialize by default' seems to be coming for javascript > [3] > (note also, Alex, that there is mention in that proposal about 'real' > private members, which you brought up recently as a possible reason > not to > access bindable private members by their names instead of via a > generated > getter/setter - because of future possible changes like this, iirc). > > General issues with conformance in class definitions > Dynamic reflection at runtime is difficult because of how we represent > classes. For the near term we probably have to stick with how things > are in > terms of how the classes are defined, but I do think we would be > better off > aiming to output conforming 'actionscript' class objects that have > non-enumerable class members (statics or instances). Doing this sooner > rather than later would be more future proof as we move to es6 and > beyond, > where achieving that will be easier in any case, I believe. > I personally think that PAYG should happen after (language > reference/reference implementation) conformance, because > non-conformance, > where it is possible to avoid, likely has a bigger cost (implication) > in > the long term (I am not talking about performance costs, but other > types of > 'cost', such as maintenance costs). 'conformance, then performance' > would > be my mantra :) Anyway, that is just airing my opinion, like I said I > expect we need to stick with how things are for now. If I get time this > year I will try to start on es6+ output. That seems to be standard in > other > areas, like React work I do elsewhere now. It usually gets transpiled > down > to es5 for IE11 and older, but as soon as those IE browsers are finally > considered unimportant, I think all the other important browsers > support > es6 already, so projects that use es6 are already prepared for that. > And I > believe GCL handles es6 to es5 as well (I think I read that somewhere). > > > 1. dynamic reflection testing: > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Froyale-asjs%2Fblob%2Fbcc4467f4febf8a279b374db86724742fada0eff%2Fmanualtests%2FUnitTests%2Fsrc%2Fmain%2Froyale%2FflexUnitTests%2Freflection%2FReflectionTesterTestDynamic.as&data=02%7C01%7Caharui%40adobe.com%7C2ea45273c0dc4e0d88e308d69ea33565%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636870821147764511&sdata=OWk0X7WRwsFgRMH8KLf5jcPfw6iTCc7yluXGzwXzF18%3D&reserved=0 > > 2. > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Froyale-asjs%2Fblob%2Fbcc4467f4febf8a279b374db86724742fada0eff%2Fmanualtests%2FUnitTests%2Fsrc%2Fmain%2Froyale%2FflexUnitTests%2Freflection%2FReflectionTesterTestDynamic.as%23L160&data=02%7C01%7Caharui%40adobe.com%7C2ea45273c0dc4e0d88e308d69ea33565%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636870821147764511&sdata=DOWN4gcWDP4iRI%2FDY4enYR3RRciPt5qULQYvnE0eP%2FM%3D&reserved=0 > > 3. > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftc39%2Fproposal-class-fields%23fields-without-initializers-are-set-to-undefined&data=02%7C01%7Caharui%40adobe.com%7C2ea45273c0dc4e0d88e308d69ea33565%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636870821147764511&sdata=8S5sGzZCbQeqNxswqk3zxJl1YB8g9erFzjceUVt%2FGOI%3D&reserved=0 > > >