You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@royale.apache.org by Greg Dove <gr...@gmail.com> on 2019/03/02 00:08:17 UTC

Reflection updates in amf_updates branch (compiler and as-js repos)

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://github.com/apache/royale-asjs/blob/bcc4467f4febf8a279b374db86724742fada0eff/manualtests/UnitTests/src/main/royale/flexUnitTests/reflection/ReflectionTesterTestDynamic.as

2.
https://github.com/apache/royale-asjs/blob/bcc4467f4febf8a279b374db86724742fada0eff/manualtests/UnitTests/src/main/royale/flexUnitTests/reflection/ReflectionTesterTestDynamic.as#L160

3.
https://github.com/tc39/proposal-class-fields#fields-without-initializers-are-set-to-undefined

Re: Reflection updates in amf_updates branch (compiler and as-js repos)

Posted by Alex Harui <ah...@adobe.com.INVALID>.
Hi Greg,

Lots of topics in here.

Regarding options for initializers, it could work the way you suggest.  It made me wonder if some option could just strip out initializers of certain types and values.  Isn't that the opposite of what js-default-initializers does?  I think the compiler reads every line of every JS file it copies into js-debug already, so I'm not sure how much a hit it will be and it will be PAYG for person who wants to try running without initializers.

The reason we think we don't need the compiler to generate initializers in the framework is because we think we can know what we are doing and know which variables really do need initialization or not.  We would manually initialize something that was causing performance issues because it is undefined. We just wouldn't have the compiler initialize all of them just-in-case.   The app dev porting their code may not be able to determine what to do when facing some problem due to uninitialized variables, and thus I think we have consensus for js-default-initializers to be on by default and turned off by updating the compile-*-config.xml files in the framework.

Google says that their minification scheme is optimized for zip compression over the wire.  I don't think we do any minification ourselves, I think we let Google do it.  The plan to shorten the decorated names of private vars should still be valid.   I don't think we plan to use really short names, but shorter than we do now to make it easier to debug.  These names will probably still be replaced by Google's minification.

If you want to experiment with patterns for having "debug-mode" checks in the framework, that would be great.  Using goog.DEBUG seems like a reasonable approach.  It would be nice to see if there are substitution patterns that could swap out beads instead of just having code disappear in production.  Substituting beads can be useful for automated testing and accessibility support.

Thanks for your contributions,
-Alex

On 3/2/19, 11:26 PM, "Greg Dove" <gr...@gmail.com> wrote:

    '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 <ah...@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" <gr...@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&amp;data=02%7C01%7Caharui%40adobe.com%7C18a73ec567d04f31aea808d69fa98c3e%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636871947912344860&amp;sdata=frqJMB971mcSqGlkWouycaL5HssdlIxq3eanNfiIVv0%3D&amp;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&amp;data=02%7C01%7Caharui%40adobe.com%7C18a73ec567d04f31aea808d69fa98c3e%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636871947912344860&amp;sdata=rebiJwnyfgWL6Ieq7LJrBmkeF3WWuDXMLgyExjK1kvc%3D&amp;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&amp;data=02%7C01%7Caharui%40adobe.com%7C18a73ec567d04f31aea808d69fa98c3e%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636871947912344860&amp;sdata=TIprNRsdcemR1o42wUnl0Xf%2Fa5ixCmuY8WxQG3hMHtw%3D&amp;reserved=0
    >
    >
    >
    


Re: Reflection updates in amf_updates branch (compiler and as-js repos)

Posted by Greg Dove <gr...@gmail.com>.
'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 <ah...@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" <gr...@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&amp;data=02%7C01%7Caharui%40adobe.com%7C2ea45273c0dc4e0d88e308d69ea33565%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636870821147764511&amp;sdata=OWk0X7WRwsFgRMH8KLf5jcPfw6iTCc7yluXGzwXzF18%3D&amp;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&amp;data=02%7C01%7Caharui%40adobe.com%7C2ea45273c0dc4e0d88e308d69ea33565%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636870821147764511&amp;sdata=DOWN4gcWDP4iRI%2FDY4enYR3RRciPt5qULQYvnE0eP%2FM%3D&amp;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&amp;data=02%7C01%7Caharui%40adobe.com%7C2ea45273c0dc4e0d88e308d69ea33565%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636870821147764511&amp;sdata=8S5sGzZCbQeqNxswqk3zxJl1YB8g9erFzjceUVt%2FGOI%3D&amp;reserved=0
>
>
>

Re: Reflection updates in amf_updates branch (compiler and as-js repos)

Posted by Alex Harui <ah...@adobe.com.INVALID>.
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" <gr...@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&amp;data=02%7C01%7Caharui%40adobe.com%7C2ea45273c0dc4e0d88e308d69ea33565%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636870821147764511&amp;sdata=OWk0X7WRwsFgRMH8KLf5jcPfw6iTCc7yluXGzwXzF18%3D&amp;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&amp;data=02%7C01%7Caharui%40adobe.com%7C2ea45273c0dc4e0d88e308d69ea33565%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636870821147764511&amp;sdata=DOWN4gcWDP4iRI%2FDY4enYR3RRciPt5qULQYvnE0eP%2FM%3D&amp;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&amp;data=02%7C01%7Caharui%40adobe.com%7C2ea45273c0dc4e0d88e308d69ea33565%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636870821147764511&amp;sdata=8S5sGzZCbQeqNxswqk3zxJl1YB8g9erFzjceUVt%2FGOI%3D&amp;reserved=0