apache / royale-asjs

Apache Royale ASJS
Apache License 2.0
370 stars 116 forks source link

Fix PAYG violations and code debt #641

Open Harbs opened 4 years ago

Harbs commented 4 years ago

We have built up code debt by not being strict enough in PAYG. This issue is an attempt to track PAYG and code inflation problems and come up with solutions to fix them.

I'm using HelloWorld to analyze and improve PAYG issues for now.

Identified issues:

Harbs commented 4 years ago

I fixed the first two problems today. That resulted in an improvement of a few KB.

Harbs commented 4 years ago

I'm not sure why we have the duplicate function definitions. I'm pretty sure this was introduced at some point and it seems to happen at the minification stage.

Harbs commented 4 years ago

I don't remember why public functions are exported. getters need to be exported for MXML support, but I'm not sure why that would apply to functions.

greg-dove commented 4 years ago

Hi Harbs,

A couple of quick comments:

Reflection data is included in debug build as it should be, but last time I checked I think it was completely omitted in HelloWorld release build. Please let me know if that is not the case.

If you have a static utils class, you can eliminate reflection and export selectively on that class by annotating the class itself with @royalesuppressexport. I did this already with js Language, for example. This approach should not be used without consideration about how it limits use of the class. But I think it would probably be low risk for sets of utility functions. It would not be advisable for the general case with a class that has public static costs for example... they may need to be accessed with string keys in release build (if used in mxml bindings for example) and therefore continue to need 'export'

On Mon, 30 Dec 2019, 01:08 Harbs, notifications@github.com wrote:

I don't remember why public functions are exported. getters need to be exported for MXML support, but I'm not sure why that would apply to functions.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/apache/royale-asjs/issues/641?email_source=notifications&email_token=ABC3PVQO5AEM6IZ4UFFAAZTQ3CHLHA5CNFSM4KA3KYFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHY56GQ#issuecomment-569499418, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABC3PVRBMV2QK454VMXPAEDQ3CHLHANCNFSM4KA3KYFA .

Harbs commented 4 years ago

Here's is some minified code which I believe comes from CSSUtils:

Df.prototype.h=function(){return{methods:function(){return{'|attributeFromColor':{type:m,g:Mb,parameters:function(){return[$b,!1]}},'|toNumber':{type:k,g:Mb,parameters:function(){return[m,!1,k,!0]}},'|toColor':{type:$b,g:Mb,parameters:function(){return[l,!1]}},'|toColorWithAlpha':{type:$b,g:Mb,parameters:function(){return[l,!1]}},'|getTopValue':{type:k,g:Mb,parameters:function(){return[l,!1,l,!1,k,!0]}},'|getRightValue':{type:k,g:Mb,parameters:function(){return[l,!1,l,!1,k,!0]}},'|getBottomValue':{type:k,
g:Mb,parameters:function(){return[l,!1,l,!1,k,!0]}},'|getLeftValue':{type:k,g:Mb,parameters:function(){return[l,!1,l,!1,k,!0]}},'|getSideValue':{type:k,g:Mb,parameters:function(){return[l,!1,l,!1,q,!1,k,!0]}}}}}};

I believe this is from:

org.apache.royale.utils.CSSUtils.prototype.ROYALE_REFLECTION_INFO = function () {
  return {
    methods: function () {
      return {
        '|attributeFromColor': { type: 'String', declaredBy: 'org.apache.royale.utils.CSSUtils', parameters: function () { return [ 'uint', false ]; }},
        '|toNumber': { type: 'Number', declaredBy: 'org.apache.royale.utils.CSSUtils', parameters: function () { return [ 'String', false ,'Number', true ]; }},
        '|toColor': { type: 'uint', declaredBy: 'org.apache.royale.utils.CSSUtils', parameters: function () { return [ 'Object', false ]; }},
        '|toColorWithAlpha': { type: 'uint', declaredBy: 'org.apache.royale.utils.CSSUtils', parameters: function () { return [ 'Object', false ]; }},
        '|getTopValue': { type: 'Number', declaredBy: 'org.apache.royale.utils.CSSUtils', parameters: function () { return [ 'Object', false ,'Object', false ,'Number', true ]; }},
        '|getRightValue': { type: 'Number', declaredBy: 'org.apache.royale.utils.CSSUtils', parameters: function () { return [ 'Object', false ,'Object', false ,'Number', true ]; }},
        '|getBottomValue': { type: 'Number', declaredBy: 'org.apache.royale.utils.CSSUtils', parameters: function () { return [ 'Object', false ,'Object', false ,'Number', true ]; }},
        '|getLeftValue': { type: 'Number', declaredBy: 'org.apache.royale.utils.CSSUtils', parameters: function () { return [ 'Object', false ,'Object', false ,'Number', true ]; }},
        '|getSideValue': { type: 'Number', declaredBy: 'org.apache.royale.utils.CSSUtils', parameters: function () { return [ 'Object', false ,'Object', false ,'int', false ,'Number', true ]; }}
      };
    }
  };
};
Harbs commented 4 years ago

We probably should be using @royalesuppressexport more aggressively and document it for users. I actually forgot about that. What are the use cases where we need to export the class names?

greg-dove commented 4 years ago

Thanks for the reflection data info. I am not sure why that is happening. it seems odd, especially if it is being selectively included for certain classes and not others. I will look into it.

class names are required for some binding cases and in all cases for reflection. I am not sure if there are other use cases.

For @royalesuppressexport I only added compiler support for that earlier this year. The beginnings of docs are here, (definitely needs attention): https://apache.github.io/royale-docs/create-an-application/optimizations/doc-comment-directives.html#miscellaneous That does not mention use at class level, because it was originally introduced as class member level only (where it also continues to work). Using it at class level makes the whole class un-reflectable and removes all exports. I have used that in a couple of places so far (and in the imminent swf graphics support).

However I think it would be a mistake to apply use of it too liberally because doing so makes assumptions that we know more about what users want than they do. I think classes with only static utility functions would be 'low risk' for using this, but even so we cannot be certain that someone somewhere will never want to use them with regular reflection support from the framework, hence they are not 'no risk'. Wherever I have used it so far I (hope I) have documented the class as being unreflectable in javascript. As a general comment, I would personally favour more approaches for optimization that are opt-in by users rather than imposed on them. For reflection data in general, one option I already suggested earlier for this could be the categorization of code (via bit flags) to allow for selective removal from release builds via goog define. Static utility classes could be one type of categorization here. Framework/Non-framework could be another, with plenty more scope for categories (the set of categories being something for collective review I think). Technically I believe this is possible, and would mean users could elect to retain reflection data for only the parts that are needed (within reason: categorization has its own 'generalizations', by definition). And by default it could allow all categories. Whether it is the best way to do this, I am not sure. But this feature could also (for example) be introduced post 1.0 because I don't think this is high priority from users (based on asking them what's important - although my discussion focus has always been on migrating legacy Flex apps with the various people I have been talking to during past several months - migrating apps is still the best opportunity for growth IMO).

Harbs commented 4 years ago

Good luck with the reflection data. FWIW, I see 49 instances of methods:function(){return{ in the code (which is a good indication of reflection data which was not optimized away).

There are a number of different cases where we're specifying @export:

  1. goog.exportSymbol on classes.
  2. public static functions
  3. public static vars
  4. public static consts
  5. For public static getters, we're using brackets to prevent renaming. (IIRC)
  6. public functions
  7. public vars
  8. public getters and setters

I think we should reexamine all of these and document the reason for each of these, so we can make intelligent decisions on when exactly it's necessary. Additionally, it might be helpful to try and figure out if there's some way to enable dead-code removal of some public members to enable PAYG on some pieces while making it easier to design and use components.

What's unclear to me at the moment is specifically item 2 and 6. Why is it necessary to @export functions (both static and otherwise).

Additionally (possibly related), I don't understand why it's necessary to include methods in reflection data.

Harbs commented 4 years ago

I added a wiki page as a placeholder for info on all of this: https://github.com/apache/royale-asjs/wiki/Minification-Notes

Harbs commented 4 years ago

Oh. I forgot one case above:

  1. Package level public functions are currently being @exported too. I can't imagine a technical reason why this should be necessary. I'm guessing this was probably never properly considered.
greg-dove commented 4 years ago

Additionally (possibly related), I don't understand why it's necessary to include methods in reflection data.

Having information about what methods are available, their associated metadata (if any), what parameters they take and what is expected to be returned by calling them is one very common use of reflection (probably on any platform). This is true of instance methods or static methods (although static methods may be less commonly used). The two Unit testing setups are one example, Crux is another. While parameters are not so relevant in unit testing (because test methods don't have any), for example, the other aspects just described are important (for both static and instance methods). Regardless of whether you use the reflection classes or not, exporting is necessary for string based access of public methods, whether they are static or instance.

The question again is whether an app uses a feature that needs that or not. Addressing that IMO is again something I think the user should have control over.

Assuming we are resigned to having a dependency on GCL, I think moving to '@export' annotations may have been a mistake, longer term.

If (for example) a user decided there was never a need for exported output, this too could be delineated in all classes with a goog define, so users could opt to avoid it. Reflection is probably the main use, but I think things like constant bindings need it too. You might remember a thread where I mentioned this type of thing in the past (being able to 'switch' options like default initializers etc) without having to recompile swcs. I think you and Josh liked the idea, but not everyone did. The same thing could be applied here: //exports section near bottom of class output if (Build.INCLUDE_EXPORTS || (some expression for reflection category match) ) { //goog.defines

//output all exports explicitly here (via goog.exportProperty etc instead of via @export annotations)

}

I personally think that type of approach where 'things work' by default, but people can switch them off easily with low effort is the better solution longer term, and also starts to make more use of GCC for what it is supposed to be good at. I can't however prioritize it at the moment.

greg-dove commented 4 years ago

Quick follow-up: It appears that issue with the reflection methods data being included in Hello World has been there for a long time. I must have missed it when I was addressing something else more obvious in the past (removing some exports that should not have been added). I was told at one point that reflection data was omitted if it was not being used, as long as it was not exported, but it seems that was incorrect. I will try to figure it out in the coming days.

aharui commented 4 years ago

I'm pretty sure I saw reflection data being removed at one point in time.

aharui commented 4 years ago

Modules is another reason that many things are exported. APIs established in the loading app are called from the module so the module doesn't have its own copy of things. This is especially important when there is static state in the shared API.

Harbs commented 4 years ago

Modules is another reason that many things are exported.

Maybe we should rethink that. GCL has the ability to output "chunks". I'm pretty sure the renaming and dead code removal takes all the chunks into account so @export is not required for that. https://stackoverflow.com/questions/10395810/how-do-i-split-my-javascript-into-modules-using-googles-closure-compiler/10401030#10401030

aharui commented 4 years ago

Briefly reading through the "chunk" link, that is not how Royale modules work. The purpose of Royale modules is to allow independent compilation of the module. The "chunk" concept seems to require all compiled code in one place.

I've spent a great deal of time overriding parts of Closure Compiler to allow more minification, but there remains a problem in TourDeFlexModules and until we solve it, it will be hard to know if we can allow for minification across modules.

Harbs commented 4 years ago

It seems a heavy price to pay to force @export on all apps for the few apps which require separate compilation for modules. It seems to me:

  1. "Normal" apps should not need it.
  2. Modules which are compiled at the same time as the main application don't need it either. (FWIW, the one time I used modules in Flex, I always compiled the modules together with the main app)
  3. Even if the modules are compiled spearately, as long as they are minified at the same time, there should be no issues.
  4. If complete separate compilation is required, then maybe minification is a price that needs to be paid for that use case.
Harbs commented 4 years ago

I think we're also conflating two things in our minification discussions. The three cases I believe renaming is an issue are:

  1. Binding
  2. Reflection
  3. Modules

For binding, the issue is that the closure compiler does not know about these binding expressions to keep the original names. However, the Royale compiler does know about these binding expressions when compiling the MXML. If we pass the responsibility to name keeping to the time of MXML compilation, we can be very specific about which members we prevent renaming on.

I'm not that familiar with reflection, but I assume/hope there's a way to deal with that. At the very least, if reflection classes are not used at all, we can know that reflection is not an issue.

Modules are obviously only an issue for applications which are compiled as modules.

To me, the question is how can we move the logic to determine what needs to be preserved to the time when we can determine it. I think this question deserves thought.

aharui commented 4 years ago

Yes, it would be great if minification was smarter. I wouldn't put all of the cost of exporting on modules. As mentioned above, it serves other use cases.

For the purpose of this attempt to put HelloWorld on a diet, I think all previous versions had exports so exported names is not the reason we grew from 20K to 60K (a couple of years ago) to 90K.

IIRC, Flex is about 130K SWF for Helloworld. I'll be happy if Royale can get to half of that (65K).

aharui commented 4 years ago

You are welcome to think about whatever you want. I first want to make sure that every line of code in HelloWorld is truly needed. But I don't even have time for that right now. I'm just sad that it has grown so much. As I just posted, all prior versions also exported names so it is additional code that added the weight.

If we can save even more later by reducing exports, that's great, but not a way to hide the fact that we're adding code that isn't needed.

If you have time to work on the compiler, I believe that we can control renaming via global object keys. IOW, even if you took away all of the @exports, the Object.defineProperties structure also prevents renaming of properties. Or you could feed some starting set of strings into the compiler pass that does the renames.

Harbs commented 4 years ago

If we can save even more later by reducing exports, that's great, but not a way to hide the fact that we're adding code that isn't needed.

Of course. I'm trying to figure out where the weight is coming from. Here's some data:

Currently HelloWorld is compiling to 102 KB (although I'm sure I was seeing 98K yesterday). It looks like my changes from yesterday are not applied on this computer, so that explains that difference.

Manually removing reflection data gets that number down to 77KB.

Manually removing virtually all exports gets that number down to 53KB. However that actually breaks the app. I think the reason for that is because besides the cases above, we need exports for event handlers. I think the only reason we need @exports for that case is because we're calling Language.closure, but I might be wrong about this.

Additionally, I see quite a bit of Vector code in the minified code. I just manually deleted that, and it looks like the Vector code in Language adds about 5KB.

Then there's the "synthType" code. and quite a few TypeErrors that did not used to be there. I don't understand the synthType code, so I don't know what's necessary there or why. I wonder if the TypeError code can be debug-only.

The dependencies on the goog event code appears to add about 8KB (not 100% sure on the exact number yet).

I'm pretty sure, we can get the total code size to under 40KB for HelloWorld if we deal with all of these identified issues, and possibly lower.

Harbs commented 4 years ago

FYI, the reason I'm harping on @exports so much is because the debt scales pretty linearly as the application grows. If it adds 25KB to an app which includes 80 classes, it almost certainly adds over half a MB to my app which has 1273 included classes.

So while it's not the PAYG issue you're primarily focused on right now, it's probably the most significant source of code bloat for large applications.

carlosrovira commented 4 years ago

Hi,

Thinks like Reflection, Metadata, Crux are the things I expect users will want for real apps. So for this ones I think Greg is right in have it by default since are the features a normal royale user will want in a framework like this (if not probably will go with simple solutions).

So For Reflection, people that will not want it should have a way to remove it completely. I think that kind of "switch" should be easily implemented, and will be an optimization technique for people for worried about it.

Modules are other important feature for any serious App (not short, tiny examples or Hello worlds), so as well should be a first citizen feature and again be optimized via some switch for people that does not use it.

I think all of this are very important things, but my opinion is that we still have some issues more important to solve for 1.0 that I'll put on priority. And I'm talking of things people will want to use and right now they can't.For example:

We are here freely, and we all can spend our time in whatever we want. All work that make we advance a bit is welcome. Just want to expose this here, since IMHO, optimizations use to come when most of the things are working. I'm with Greg that for me most of this are post 1.0 issues to improve. But I don't think this will make a difference in Royale positioning or market penetration.

I still didn't find a user or a client complaining me about size, or any optimizations (that does not mean are important or something we shouldn't pursue, don't interpret me bad, I think all is important). They complain always about components still not working (or still not created), features not implemented and most of the time issues with Bindings that worked in Flex code and not in Royale (I think Bindings is the main source of frustration in a migration) or they trying to add some CSS rule that makes compiler fail to compile.

Harbs commented 4 years ago

FYI, I find no need for Modules in any of my serious apps, nor reflection, or compiled metadata (I only run tests on debug builds).

I personally have an aversion to "magic" in programing so I never felt an attraction for fancy dependency injection frameworks.

My app has increased in size from about 1.5 MB to over 3MB in the last year and a half.

While some of it is due to added code, a significant amount (I'd guess somewhere in the range of 1MB) is due to unnecessary code bloat from these features.

So for me, yes, this is very important to solve.

piotrzarzycki21 commented 4 years ago

Thinks like Reflection, Metadata, Crux are the things I expect users will want for real apps. So for this ones I think Greg is right in have it by default since are the features a normal royale user will want in a framework like this (if not probably will go with simple solutions).

There is no statistics what user want or not and we are too small to gather any. Crux, Reflection to me it is definitely some addition and shouldn't be default part of any output in case of Hello World. If this is happen now - I'm +1 to remove any of that from Hello World. In Prominic we will definitely in the future use Crux, Reflection etc. in upcoming projects, but I don't see any value to have any part of it as default switch ON.

Harbs commented 4 years ago

@carlosrovira One thing I would like to solve which I believe is important to you is PAYG public functions in Base classes.

Right now, there is a class of problems which is hard to solve in a user-friendly PAYG way. One example is your "reset-width-to-default" problem. It's not a problem that many applications have, but if we don't put it into UIBase, it's hard to reset the base values using beads. If we could enable a method which is there if you use it, but would disappear from the output code if you don't, we'd have the best of both worlds.

I don't know that we can solve this problem, but I think it should be possible, and I'd like to try...

I think we need more tools in our PAYG toolbox... 😉

carlosrovira commented 4 years ago

In Prominic we will definitely in the future use Crux, Reflection etc. in upcoming projects, but I don't see any value to have any part of it as default switch ON.

Yes, I'm for removing for small examples (like Hello World), making some switch to "opt-out". But I think we should have as default since, as you said, for normal projects that will be (normally) something people would want to use.

Hello world, blog examples, tutorials, use to be learn materials, to know how to do a concrete thing, but people will concentrate in make a full app, and that means using Crux, Metadata, and introduce lots of components and use case that needs that tooling.

carlosrovira commented 4 years ago

If we could enable a method which is there if you use it, but would disappear from the output code if you don't, we'd have the best of both worlds.

Sounds very interesting, but I think @joshtynjala commented some time that GCL was doing that for us now. I think was in a thread where other user was talking about remove code by the compiler... Maybe I'm wrong?

Anyway I think that's interesting to pursue too. I'm a fan to get optimizations as tooling (Metadata, Injection) and new language features (like generics,...) and even mixed things like AOP. But as well I think we need to do all with some order in mind, since we need people that like Royale stay with us, and can only be done if we all know that all that people have needs and requirements to use Royale for his real work. If we fail in recognizing the priorities we can be working for something that people will like but never use since does not solve the basics of their needs.

So again, I love all of this, but since I can't say you all nothing about what we should do, just can recommend you think in the list of priorities this project can have and if is worth to fight to get it done to get more users on board.

just my 2

Harbs commented 4 years ago

Yes, I'm for removing for small examples (like Hello World), making some switch to "opt-out". But I think we should have as default since, as you said, for normal projects that will be (normally) something people would want to use.

To me the right compromise is to have configurations which have compiler options set to a group of defaults so we don't have to have these discussions on what the "right" set of defaults are.

Harbs commented 4 years ago

Latest commit gets rid of Vector code in Language. That eliminates 8KB

aharui commented 4 years ago

PAYG implies opt-in, not opt-out. If you use it, you pay for it.

If we don't adhere to these principles, then we'll make the same mistakes as in Flex and it won't save anyone anything when they try to opt out because we won't have separation of the implementations of the APIs.

What will be opt out is optimizations. Once you've chosen your set of APIs to create a fully functional app, you can choose to remove runtime type-checking and other things you may not need.

Harbs commented 4 years ago

I just discovered the goog.reflect file which has the following interesting looking functions:

/**
 * Syntax for object literal casts.
 * @see http://go/jscompiler-renaming
 * @see https://goo.gl/CRs09P
 *
 * Use this if you have an object literal whose keys need to have the same names
 * as the properties of some class even after they are renamed by the compiler.
 *
 * @param {!Function} type Type to cast to.
 * @param {Object} object Object literal to cast.
 * @return {Object} The object literal.
 */
goog.reflect.object = function(type, object) {
  return object;
};

/**
 * Syntax for renaming property strings.
 * @see http://go/jscompiler-renaming
 * @see https://goo.gl/CRs09P
 *
 * Use this if you have an need to access a property as a string, but want
 * to also have the property renamed by the compiler. In contrast to
 * goog.reflect.object, this method takes an instance of an object.
 *
 * Properties must be simple names (not qualified names).
 *
 * @param {string} prop Name of the property
 * @param {!Object} object Instance of the object whose type will be used
 *     for renaming
 * @return {string} The renamed property.
 */
goog.reflect.objectProperty = function(prop, object) {
  return prop;
};

I wonder if they might be useful for data binding and other cases where we need string access.

aharui commented 4 years ago

Regarding @export. I do not think it is as expensive as you claim. If you have:

org.apache.royale.html.Basic.prototype.foo = function() {trace('foo')}
org.apache.royale.html.Basic.prototype.bar = function() {trace('bar')}

I'm pretty sure in js-release it looks something like:

zz = {trace('foo')};
bb = {trace('bar')};
export([a,b,c,d,e,f,'foo',zz);
export([a,b,c,d,e,f,'bar',bb);

And in many cases method bodies totally outweigh that code. Strings are put in a table and the exports use an array of string references.

aharui commented 4 years ago

I do believe that an export eliminates dead-code removal, so by removing more exports, we should see more code get removed. That is a tool in the toolbox we have not leveraged so far.

But again, I would like to test apples-vs-apples and without changing how we handle @exports, see how much code we can kick out.

aharui commented 4 years ago

If your clients have never run into size/performance issues that is not too surprising. It is why you were a fan of Flex. Flex did not fail you. But it did fail others and I do not wish to fail them this time around.

Harbs commented 4 years ago

But again, I would like to test apples-vs-apples and without changing how we handle @exports, see how much code we can kick out.

Absolutely. That's what I've been doing.

So far, I've gotten rid of 12KB from HelloWorld without touching @export, without solving reflection data, and without removing the goog.events dependencies.

carlosrovira commented 4 years ago

To me the right compromise is to have configurations which have compiler options set to a group of defaults so we don't have to have these discussions on what the "right" set of defaults are.

I'm too. I think most of the possibilities would end in some of the next configs:

Maybe we can have other configs for that. We'll get as well improvements in IDE experiences. For example: If I just work with MX/Spark, I don't want to see Jewel things, and the same for other cases...

Harbs commented 4 years ago

I spent quite a bit of time looking into the dead code removal issue.

It looks to me like dead code removal is working for static Royale methods, but it does not seem to be working for ones written to the prototype. I manually removed @exports from some classes and all the methods remained in the minified code. I don't know why the dead code removal is not working for prototype functions. I hope someone else can figure it out...

aharui commented 4 years ago

Regarding dead-code removal of prototype methods: I'm not sure how you tested, but it might be that the reflection info fools the dead-code remover by implicitly referencing the methods. If you haven't, I suggest testing with some vanilla JS and see what Closure Compiler can do.

Harbs commented 4 years ago

It's definitely supposed to do dead code removal of prototype methods, but in practice, it isn't doing so in HelloWorld (or my own app AFAICT).

I initially suspected there was something about the structure of the reflection data, and I tried many different things there. I started suspecting that something else was going on, so I removed @export on all removeElement methods in the JS files. removeElement is not used anywhere in the app, so theoretically getting rid of @export on those methods should have made the dead code removal effective on those methods. Removing @export had no effect.

Like I said, I edited the JS files. To minify, I used VSCode to do a release build without first cleaning the project, so the modified JS files should be the ones being used.

I hope this info is useful to someone else, because I don't know where to look at this point...

aharui commented 4 years ago

I suggest trying a very simple test case without Royale and calling closure compiler from the command-line to verify what it can do.

The Royale compiler sets a ton of options on the closure compiler and could be setting something that screws up removal of prototype methods.

There is a Royale compiler option '-skip-transpile' that is intended to skip any JS code generation and just run the closure compiler on bin/js-debug. Otherwise the source files get re-transpiled and remove-circulars runs on the output js and maybe that screws up dead-code removal (although it doesn't seem likely)

Harbs commented 4 years ago

I got it to work by doing the following:

  1. Removed @export on removeElement from Application, GroupBase, IParent and UIBase.
  2. Removed the Reflection data from all of those files manually.

Doing so removed the removeElement code completely from the compiled app.

Both of these steps were required. If the reflection data was left in any of those files, dead code removal was not enabled. There's clearly something about the the reflection data which is preventing code removal. What's also interesting is that if @export was left in in any of the above classes, the function code for all of them was not removed. I'm not sure why this is.

Harbs commented 4 years ago

FYI, manually removing reflection data and @exports from IBorderPaddingMarginValuesImpl and SimpleCSSValuesImpl resulted in a further reduction of 5KB -- from only two files! Strategic removal of more @exports and goog.exportSymbol can remove dependencies to things like EdgeData and MarginData which are not used in HelloWorld at all.

greg-dove commented 4 years ago

The issue with Reflection data being 'held' is fixed and was something GCC related to do with the way I added some new data items at one point (which were not being exported). Maybe it is something that works in a more recent version of GCC. There was no self-referencing or external referencing involved, so I'm not sure why it was treating it as if there was something like that. Either way I was able to avoid it with a refactor of some specific Reflection support data. The original data was set up as a nested part of the original reflection data function/object. The change was to simply add it directly to prototype as extra data fields. Because these are only there to support specific reflection needs they will still be eliminated from apps that use only Reflection functionality that doesn't need this specific data. Anyway, good call Harbs, thanks for drawing attention to this.

slightly OT: I think there is still scope for future improvements for users of Reflection itself as I mentioned earlier, but IMO this focus ('must be smallest possible') is not what most people think is a priority and so I think it is just something to do 'when I can get to it'. I believe Royale output already compares favorably to other options. Based on actually dealing with credibility challenges when talking to prospective clients, people are most keen to see their business logic work like it did before and following on from that, can be sold on components still needing 'some work' (and possible contributions from them being required to achieve that). Unless we start growing the community very soon I fear it will be too late. That is the most critical thing this year if Royale is to gain some traction and to ultimately have some outcome of success, in my mind. One of the biggest impediments to confidence in choosing Royale is the small number of developers. It's more about the long term ramifications than the short term availability of those of us who (like me) who are available. I believe 1.0 is a starting point for that and it would be great if we could all come to some agreement on that. I might be wrong but I think people generally agree, perhaps with some reservations, or at least don't disagree with the idea. Alongside that it would be good to collaborate on a collective shortlist of what is vital to get there (not what is 'ideal'). I'll bring that up as a topic on dev list soon (feel free to do so yourselves if you want).

another OT: I would be happy to try to update to latest GCC (in a branch) at some point (again, not something I consider a priority now that the Reflection thing is resolved) but want to check one thing - are we still stuck on Java 7 SDK minimum for everything or can we up the minimum to 8? GCC has 8 as a minimum, and I see in the past that Alex has back-ported some GCC code with the local Config variation work in 7. This could get increasingly more tricky I think with the use of lambda expressions in the more recent GCC code, although I am still learning-as-I-go on this stuff.

piotrzarzycki21 commented 4 years ago

Guys I just wanted to say that optimization which has been done so far decrease significantly size of our release version of application. Application 1 from 1.68 MB to 1.35 MB! Application 2 from 860 KB to 620 KB

carlosrovira commented 4 years ago

Hi @greg-dove, I think actually min version is Java 8 since we have already things using it like SASS maven plugin and other maven features. In the other hand current java statistics marks Java 8 as the actual version used by 83% of users [1], while 7 and 9 are 13% and 14% respectively.

About priorities. I'm with you that we should prioritize our efforts to target 1.0. I think Docs, fix Bindings issues, MX/SPARK Components and fix cumbersome bugs (like CSS support) should be critical to all of us. Of course, all people involved is free to work on whatever they want and we can just ask about it.

Looking at the past 6 months in the website statistics we had a peak this past October (2.800 sessions) coming from July and August with 2.300-400 sessions. Then November and December fall around 2.200. We should pay attention to it we if we care.

[1] https://www.jetbrains.com/lp/devecosystem-2019/java/

Harbs commented 4 years ago

Great work! I'll confirm the changes with my app tomorrow. 😄

Minimum of Java version 8 should be fine IMO. I think there was a Flash Builder limitation, but IIRC, it's possible to get FB to work with version 8. Someone @aharui or @yishayw ? please confirm.

@carlosrovira I created #648. We should fill that out and agree on what constitutes a "version 1" release.

Harbs commented 4 years ago

OK. With the changes so far, HelloWorld is now down to 66KB. That's pretty close to the 65KB number @aharui suggested.

Gzipped it's 19KB

Great progress! 😄

carlosrovira commented 4 years ago

Hi! Congrats on that numbers! Good work on that improvement! :)

Minimum of Java version 8 should be fine IMO. I think there was a Flash Builder limitation, but IIRC, it's possible to get FB to work with version 8. Someone @aharui or @yishayw ? please confirm.

I think, FB should not be a barrier at all. It's an obsolete IDE and not supported since many years ago. I understand that some people would want to continue using it, but we should never break the Royale evolution for an obsolete-not supported IDE. In the other hand, maybe actual OSs will not support it (i.e: OS X Catalina?).

aharui commented 4 years ago

I am not an Java expert. AIUI, there is a minimum JDK and there is a code-style option, and a output byte-code version option for the Java compiler.

I think we've set the minimum JDK at version 8 because of SASS. But our code is still written for Java <8. I do not know what the output version is, but I think it may also be <8.

FB has a patch that will get it to run on Java 8. I do not know what would happen if the jars that interface with FB were given a newer code style and/or output version.

FB runs on OS X Sierra. I have not switched to newer. I've heard some complaints, not sure if they were resolved or not.

If someone has the time to upgrade the code style and output options and get everything working again, that's fine with me.

Updating to a new GCC is fine too, but will likely blow up the modules work I've done for js-release as I noticed they've rewritten some of the compiler passes I had to fork. So upgrading GCC is likely to be a lot of work.

IMO, unless JDK versions are being obsoleted in a way that will affect us, we have higher priorities than upgrading. But if someone wants to do it, I won't object.