Class instances in statics fails in 0.8 (BZ#1455)

[qx-bug-importer]

Gaëtan de Menten (ged) wrote:

For example, a dateFormat instance produce the following error:

qx.locale.Manager is undefined DateFormat.js (line 47) this.__locale = qx.locale.Manager.getInstance().getLocale();

See attached test script (which worked fine in 0.7.x).

assigned to Thomas Herchenroeder (@thron7)

[qx-bug-importer]

Gaëtan de Menten (ged) wrote:

Test script demonstrating the problem

ERROR: no way found to migrate Application.js (application/x-javascript) from bugzilla.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

If instantiation is used to initialize a static member, the generator in 0.8 sorts the instantiated class before the referencing class correctly, but doesn't try to recurse on run-time class dependencies of the referenced class. In 0.7 this was tried and worked for many cases, but could also end in circular references in others, where a partial order for loading classes could no longer be established. Therefore, the strategy was abandoned for 0.8 and the current policy is to not use 'new' or static method calls to initialize static members. (The scope of this policy stretches even further and includes other forms of "requiring" like the 'defer' section of a class; see the new section of the Anti-Patterns wiki page http://qooxdoo.org/documentation/0.8/antipatterns#abundandly_requiring_other_classes). So the current answer to the mail list posting is, yes, this idiom is not supported in 0.8.

We will re-evaluate the older strategy of "going-as-far-as-possible" to see if it makes sense to re-establish it in the 0.8.x line of code. Still, the recommendation to avoid requiring classes, as expressed on the wiki page, will remain valid nevertheless, since there can always be cases where the dependency relation cannot be solved completely, and automatic dependency resolution might affect parts/package building negatively.

Thomas: Check the implementation of re-prioritizing run-time dependencies in required classes in 0.7. After that, the bug has to be re-evaluated.

[qx-bug-importer]

Andreas Ecker (@ecker) wrote:

Untarget 0.8.2

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

to 0.8.3

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

* BZ#2249 has been marked as a duplicate of this bug. *

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

There is currently no general solution (more on this later), but there seems to be a work-around.

Here are the steps to reproduce the problem in a standard 'gui' skeleton:

- config.json:
    "build-script":
      "include"  : ["qx.lang.Object"],
      "variants" : { "=qx.debug" : ["on"]}
- generate.py build

qx.debug=on will trigger many code snippets where statics of qx.core.Assert are called. qx.lang.Object is a class that has such code snippets.

By listing qx.lang.Object in "include", it will be prepended to the "include" that comes from base.json. The result looks like this:

"include" : ["qx.lang.Object", "gui.theme.Theme", "gui.Application" ]

This causes a re-ordering of qx.lang.Object (which would be included anyway!) in the resulting output, together with other classes that actually call qx.lang.Object methods as static initializers (e.g. qx.lang.Core), which triggers the problem.

The work-around is to re-order the "include" key. To overcome the standard prepending of arrays during job merges, the entire list of include entries should be given in the config.json (overriding the base.json key):

- config.json:
    "build-script":
      "=include"  : ["gui.theme.Theme", "gui.Application", 

"qx.lang.Object"],

(Mind the leading "=" in front of the "include" key).

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

I have written an initial version of a method that follows dependencies of a static method if it is called as an initializer (DependencyLoader.getMethodDeps()). This method does not follow the qooxdoo class inheritence yet, to resolve methods.

Still, we cannot put this method into production, since even with this simple implementation the framework doesn't compile anymore. It quickly hits circular dependencies among framework classes. For example: qx.lang.Object uses qx.core.Variant.select for initialisation, as in

isEmpty : qx.core.Variant.select("qx.client", ...

But qx.core.Variant.select calls out to a qx.lang.Object methods, as in

throw new Error('No match for variant "' + key + '" in variants [' + qx.lang.Object.getKeysAsString(variantFunctionMap) + ...

So the framework dependencies have to be cleaned-up first, before we can procede with this extended dependency analysis.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

In order to better support refactoring of class dependencies, I have worked on improved reporting on those.

• In generator.Generator, renamed some methods to have a more unified naming scheme for reporting functionality under the 'log' banner:
  • renamed runDependencyDebug -> runLogDependencies
  • renamed runUnusedClasses -> runLogUnusedClasses
• Made the 'log' config key an action trigger for the main run() routine, so it can appear alone in a job.
• In runLogDependencies, add logging of both 'using' or 'used-by' dependencies for classes.
• Enhanced runLogDependencies to either make console print-outs of dependencies, or create a dependency graph as a .dot file (currently for 'using' dependencies only).

With r18978.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

DependencyLoader.py: I have added an extended version of getMethodDeps(), getMethodDeps1(), which follows the qooxdoo inheritance chain (both super classes and mixins) when looking up method definitions. The found method definitions are then exploited for further dependencies. The biggest issue I still see here is probably the cycle check, which tests against the signatures (class id and method name) seen so far, which might depend on the signature with which we started. This would mean the dependencies calculated for some downstream signature could get wrong, and cached wrongly. Not sure how to resolve this.

The consuming function, DependencyLoader._analyzeClassDepsNode, still only uses the class names for dependency tracking, not the method names, but this could be exploited at a later time.

Config: To activate the use of getMethodDeps/getMethodDeps1, the config key 'dependencies/follow-static-initializers' : true has to be used. But the whole thing is still highly experimental, and hits at the circular dependencies among framework classes, as mentioned in an earlier comment. So it is not usable for production.

With r19361.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

down prio for now. the bug basically depends on the framework clean-up (#2426).

[qx-bug-importer]

Andreas Ecker (@ecker) wrote:

moved from 0.8.3 to 0.9

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

down prio, as there seems little activity in the framework

[qx-bug-importer]

Andreas Ecker (@ecker) wrote:

mass renaming of 0.9 target to 1.0 (for issues with status "NEW")

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

to 1.0.1

[qx-bug-importer]

Andreas Ecker (@ecker) wrote:

moved to target 1.0.2

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Commited a working version that solves the sample application issue (see qooxdoo-contrib/Bugs/1455), using the follow-static-initializer config setting.

No circular dependencies in the framework are hit by this app, good. Have to test the other apps for that.

Also, there are no optimizations done for this solution. Esp. cache adaption has to be done.

With r23455.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

I'm closing this. The current implementation follows recursively static initializers and other occasions where immediate function application "lifts" run-time dependencies to load-time dependencies. The cache invalidation of this information has been enhanced to account for changes in class files that have been added as such recursive dependencies. The performance is quite good, although the transitive load dependencies are calculated traversing parse trees. The only thing here to watch out for is recursion depth.

I had a shot at an alternative implementation (Class.dependencies1, under the "New interface" heading), but it turned out to be awefully slow (2x slower on empty cache, but in the range of 30x slower on full cache!). The idea was to cache only "shallow" dependencies for each class (dependencies that are immediately apparent in its source code). But not in the form of a few lists, as it is now ('load', 'run', 'ignore', ...), but in a more structured way that allows you to access the dependencies of an individual class feature (like a method), rather than just on the summary level. These shallow dependencies would be easy to validate in the cache (freshness just depends on the corresponding class file). When calculating transitive load dependencies, only the shallow dependencies of each involved class have to be inspected, picking the ones for the specific method I am interested in, and then follow those dependencies. This way, transitive load dependencies would be generated everytime afresh, which I thought would be a minor overhead given the structured cache objects available for each class.

Unfortunately, I was wrong. I might do some profiling on this implementation if I find time.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

With r23524.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

I'm re-opening this, as I found a couple of issues:

• The 'defer' section was not included at all, in the transitive deps tracking; this has been fixed. Thanks to the added 'defer' tracking, other issues surfaced.
• When looking up class parents, the 'include' member might be wrapped in a qx.core.Variant.select call; this is now handled.
• When tracing recursive dependencies, references to methods were always followed, even if the method was not called (e.g only passed as a parameter); method dependencies are now only included if the method is actually called. (Of course this misses the case if the passed method is called in the function where it is passed to, but that's a completely separate story, in terms of tracking this).

The current version fails to cache results of recursive deps tracking, as this has to be re-thought. You cannot just cache results for e.g. a method when those deps are treated differently depending on whether the method reference was actually a call or not.

The current version also spits out a lot of "Skipping unknown class dependency:..." messages. Ignore this for now, I will tend to that soon.

With r23616.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

The current implementation does not cover these use cases:

• Properties: This affects setter/getter in general, but particularly dependencies in "apply" methods of a property setter which are not being picked up.
• Features added in the constructor: Features not being present in the class map, but added in the constructor with 'this.<feature> = <value>' are not being picked up. This is particularly relevant when <value> introduces further dependencies (e.g. is a function definition or class instantiation), and a derived class uses <feature> in a context that needs recursion (e.g. static initializer). Then the dependencies of <feature> would be missed.

Those cases might lead to unfullfilled dependencies showing up at load time in the client.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

> The current implementation does not cover these use cases: > > - Properties: > This affects setter/getter in general, but particularly dependencies in "apply" methods of a property setter which are not being picked up.

An example of this is qx.data.controller.Form, which uses setter methods in its constructor. Both, setModel and setTarget, run through apply methods.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Closing this bug again, as the remaining issues seem to be settled now.

With r23645.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Implementation note: Class-level dependencies

The current implementation of the recursive analysis only includes the function body dependencies of remote methods. But it is clear that a remote method (ie. method not implemented by the current class) also requires the load-dependencies of the remote class itself (ie. dependencies from static initializers, defer section, asf.).

In the current implementation, this omission is feasible. Method dependencies are projected to class dependencies, so whenever a method requires a remote method, the remote class will be required. When the remote class is later included in the class sorting, its load dependencies are automatically honored, so they don't have to be in the require list of the using class. Important for the recursive analysis is the "lifting" of dependencies that were run-time deps, to load-time deps, not load-time deps that already classified as such.

There might be circumstances in the future where this changes. Maybe at one point these class-level dependencies have to be included explicitly in the recursive method analysis, to make sure all these deps are treated. I don't think that static method removal (bug#4028) will require that already, as in this case still the entire class is loaded, modulo some unused static methods, so the automatic treatment of class-level deps still suffices.

[qx-bug-importer]

Derrell Lipman (@derrell) wrote:

Thomas, I'm not sure if my question is relevant to this issue, but seems related...

Let's say there's a large class A with many methods and statics. Now we have a method of some unrelated class B. The method of class B does not instantiate a Class A object, but does access one of Class A's static members. It seems that ideally that should cause the inclusion of the needed, single static definition from A into the build, but exclude all of the methods (and other statics) from A.

(This seems somewhat of the reverse of what you're discussing.)

Derrell

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Derrell,

> Let's say there's a large class A with many methods and statics. Now we have a > method of some unrelated class B. The method of class B does not instantiate a > Class A object, but does access one of Class A's static members. It seems that > ideally that should cause the inclusion of the needed, single static definition > from A into the build, but exclude all of the methods (and other statics) from > A.

I think this is what bug#4028 is all about. If this particular static member you are talking about were all that is ever used from class A throughout the application, you would still need to include the static with an enclosing class, as we cannot currently handle bare class features on their own, but only classes. So you would still have a 'qx.Class.define' above it, and the recursive dependencies of the static, and prob. the defer section and its dependencies, as it is hard to guess what the defer is good for. So you are basically talking about taking the original class A and remove features that turn out to be unused. bug#4028 addresses this with some 'mark-and-sweep' approach, marking all statics that are used throughout the application, and removing the remaining. The real difficulty lies in finding out what is really used :).

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

We discovered an additional case where run time dependencies are "lifted" to load time dependencies, besides initializing class map members and the 'defer' section:

• Event handlers, whos event is fired before all classes are loaded.

Event handlers are mostly methods, so they only generate run-time dependencies. If an event handler gets registered early and the corresponding event is fired before all classes are loaded, run-time dependencies of such handler might not be loaded yet. They are lifted to "quasi-load-time deps". The issue largely depends on when the corresponding event is fired, which in turn can largely depend on the browser.

Unfortunately, there are cases where the registration of such an event handler is totally opaque for the generator, e.g. in the case of qx.event.handler.Window, which registers a native load handler through its constructor. But the constructor is called in event.Manager, looping through
an internal list of registered handler classes using "new clazz(this)".

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

It appears that I've overlooked method calls in top-level position (like the very obvious qx.Class.define(...);) as load time references that require recursive dependency analysis.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Added checking of top-level function calls to the recursive load deps analysis.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

The recursive checking of top-level function calls incures a time penalty that is most visible when building Demobrowser.

There is a mild time penalty (~12%) when calculating the dependencies (cache misses), due to more occurrences that are now being checked.

But a more severe penalty shows up afterwards, on reading the cached dependency information from cache (cache hits). It appears that (un)pickling the larger amount of DependencyItem() class instances is responsible for the addtional time. The number of DepsItems per application is roughly 4-fold, as is the time spent in cache reads.

This shows in Demobrowser, where building a demo (that nearly always gets its deps from cache hits), takes approx. 0.4 secs longer on my machine. Given roughly 750 demo apps to build, this amounts to 5min it takes longer in total, which is like 55% slower for me. This goes on top of the initial 12%.

I'm leaving it at that for the moment, as I don't see a good way to work around that. I need the information in the DepsItems (especially with regard to future method pruning). I don't think it is of any gain to do the (de)serializing by hand, so that the classInfo map would only contain strings (which would probably be faster for Pickle); I would re-instantiate the DepsItem objects myself, just what Pickle does, and Pickle does it in C.

With r25725

[qx-bug-importer]

Martin Wittemann (@wittemann) wrote:

Closed all bugs already shipped with a release.