Privateoptimizer: Variable capture in derived class (BZ#2391)

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

It appears that a sort of variable capture can happen accross classes when optimizing privates. If one class has a private member that is optimized to, say, a, and per accident another private member in a derived class is also optimized to a, the old meaning of a in the superclass is lost. When methods of the superclass get called that make use of a, they fail.

This has been observed in the method qx.core.Object#addListener, which makes use of a private member of qx.event.Registry type. This member got overridden by private optimization in qx.event.Timer, and addListener could not be executed anymore.

This has been observed when compiling all classes of an application separately, and then "glueing" the compile results together. Switching off private optimization fixes it. In the generator/TreeCompiler, privates get cached and re-loaded into the privateoptimizer. This might play a part.

assigned to Thomas Herchenroeder (@thron7)

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

The private optimizer builds unique keys for the combination of the classname and the member name e.g. "qx.Class.foo" maps to "a". There should not be any issues with classes getting the same "compressed" name. This may though, be happen when something in the handling of class compilation is modified. The current code there is not really nice and depends on the correct loading and saving of the cache file. When there might be changes to allow "threaded" class compilation, this concept may break.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Ah, right, thanks.

[qx-bug-importer]

Stefan Volbers (volbers) wrote:

I believe I just stumbled over this bug. It appeared when the build optimizer changed an object's var name to "e". A listener following in the script uses an event data variable named e, which seems to not get checked or renamed by the optimizer if it is a one character name; so the names collide in the listener's callback:

var e=new Z.a(); var Y=ba.addListener(x,function(e){f.removeAll(); f.add(e.getWidget(),{left:100,top:50}); },this); // grep "e" *

I guess that would be resolved if one character variable names there would be managed/renamed too. Btw are these never renamed?

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

Stefan: What you seem to mean is the variableoptimizer, not the privateoptimizer. Privates are direct entries in "members" or "statics" and start with a double underscore "__".

[qx-bug-importer]

Stefan Volbers (volbers) wrote:

Sebastian Ouch. Indeed. I came here searching bugzilla for variable optimization issues. "addListener" and stuff sounded so well known... seems like I was not paying that much attention to details at 5am...

• sorry 4 the noise - Plz redirect me if there is an open bug for what I found; or is that new so I should open a fresh one?

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Stefan: Yes, please open a new bug for this. Please also attach a suitable part of the original source code, together with the optimized version of it. It would be ideal if you could provide a stripped down version of a skeleton file that reproduces the issue, but this is often not possible with optimization bugs. Make sure you select the qooxdoo version you are working with, and assign it to me.

Which qooxdoo version are you working with? Current qooxdoo (0.8.2) doesn't have a .getWidget() method, but 0.7 used to have!? Or are you using legacy classes in 0.8?

[qx-bug-importer]

Stefan Volbers (volbers) wrote:

Thomas done, http://bugzilla.qooxdoo.org/show_bug.cgi?id=2469 It is 0.8.2 SDK. The getWidget method is the getter to a widget property in a custom class.

[qx-bug-importer]

Christian Hagendorn (@Hagendorn) wrote:

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

[qx-bug-importer]

Christian Hagendorn (@Hagendorn) wrote:

Increased priority and severity.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

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

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

As mentioned in the other bug it seems that the call to load() of the privateoptimizer is completely missing.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

> As mentioned in the other bug it seems that the call to load() of the privateoptimizer is completely missing.

Good point! The call to load() was always there, but due to inconsistent cache id's the privates cache object was never found in load().

Fixed with r19567.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Private optimization has been re-enabled in the demos (table/Table was failing recently due to this issue), in order to test the reported fix.

But I'm not closing this bug, since it was reported on May 15, whereas the fix concerns a change introduced on May 26, so this can hardly be the (only) cure. We'll have to see if the bug re-appears in the near future...

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

> But I'm not closing this bug, since it was reported on May 15, whereas the fix concerns a change introduced on May 26, so this can hardly be the (only) cure. We'll have to see if the bug re-appears in the near future...

The mentioned change is documented in bug#2398 (adding uniqueness features to cache objects).

[qx-bug-importer]

Andreas Ecker (@ecker) wrote:

Thomas, I think you misinterpret the dependency on the date of the initial bug report: on May 15 another bug was reported and was moved to BZ#2469, so I guess we could close this one? If not, feel free to reopen.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

> Thomas, I think you misinterpret the dependency on the date of the initial bug report: on May 15 another bug was reported and was moved to BZ#2469, ...

I don't think so. The entry that resulted in bug#2469 was from Jun 10. But anyway, I think closing is ok since I haven't had any new reports of this bug since committing the fix 12 days ago.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

I've added a test script (grep_privates.py) to qooxdoo-contrib/Bugs/2391 that can be used to detect suspicious re-definitions of privates in build .js files.

[qx-bug-importer]

Daniel Wagner (@danielwagner) wrote:

Seems like this problem still exists in 0.8.3. See BZ#2832.

[qx-bug-importer]

Daniel Wagner (@danielwagner) wrote:

Set target milestone to 0.9

[qx-bug-importer]

Daniel Wagner (@danielwagner) wrote:

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

[qx-bug-importer]

Andreas Ecker (@ecker) wrote:

Too bad this shows up again, raised relevancy. But good to have a real-life example (Daniel/Werner should have all the info).

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Just to add another scenario where the variable capture can happen:

It's not only if a derived class captures a private of a super-class. It happens also when a derived class includes a mixin, and that mixin overwrites a private of the including class' super-class.

Seen in qx.ui.tree.Tree, which inlcudes qx.ui.core.MContentPadding, which overwrites a private in qx.core.Object, which is a super-class of qx.ui.tree.Tree.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

I think I now better understand how variable capture of private-optimized classes can occure. Basically, it is because of using the cache across multiple applications.

Take a framework class. It's been used in the build of application A (like the Feedreader). The compiled version, including private optimization, is written to the cache. When the next application B (e.g. the Demobrowser) needs this class in its build, the cache version is used, as it is still fresh. But other classes of application B have to be compiled and private-optimized, so the privateoptimzier only sees their privates as they are being optimized, and takes care not to assign a replacement identifier twice. But it cannot regard the private replacements that have been used for the already compiled classes. The privated cache db that is being used is specific to the application, and those classes have been optimized in the context of another application (namely application A).

I see roughly two directions to address this issue:

• Continue to use cached private-optimized classes for builds across applications. This ensures a maximum of cache reuse. But to battle the privates variable capture, you would have to do probably a lot of variable replacement on the cached objects, to replace private identifiers used in a previous build run with those compatible with the current. In short, you'd have to post-process the compiled classes. To achieve that, you would have to scan all involved cache objects prior to using them, and check their privates against a current privates db and replace them as necessary, so there is some linear overhead for cached objects.
• Or, don't cache private optimization at all, but do it afresh whenever a class is used for a build. You could still cache the compiled version of the class, including all other optimizations except for the private optimization. Private-optimization would then be done repetitively, always when the current build requires it, and the result will not be cached. This would incure a linear overhead in the number of cached objects, as they have to be re-optimized for privates every time. But the optimization itself has no overhead over the replacement necessary in the first alternative in the case that you really have to replace an already optimized symbol; if you don't have to replace it, the first alternative is cheaper.

My gut feeling is that the penality for the second alternative is not much more than that for the first on average, so I would opt for the second solution. It seems cleaner and would be easier to implement in the current generator.

[qx-bug-importer]

Fabian Jakobs (@fjakobs) wrote:

I would opt for the second option. Private optimization is in essence a global optimization. I think we will always have problems if we pretend we could do it on a per file level.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

> I would opt for the second option. Private optimization is in essence a global optimization. I think we will always have problems if we pretend we could do it on a per file level.

Thanks. Both alternatives actually acknowledge that it is a global operation. They just attack the issue from different angles (fixing an existing optimization or always optimize from scratch).

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

One moment please. Private optimization works globally. It uses a global dictionary of used words where these are build from the name of the private plus the class name where it happens. This is afterwards renamed to some random name. If this file access works, there should not be any problem. Every private field in every class should be seen as unique and should result into a unique crypted private name.

In the source version overwriting of privates should not be allowed. This is easily doable in the Class.define and is as far as I know already implemented.

It seems I still not understand what the issue here is.

I think this from comment #23 these are basically not right: "so the privateoptimzier only sees their privates as they are being optimized" and "But it cannot regard the private replacements that have been used for the already compiled classes.". The privates are cached cache-wide. So even if you have multiple applications using the same cache there should not be any issue.

Normally the privates are cached the same way as the compiled classes. It is already a global operation - just to use the same wording. If the compiler compiles a class it directly refreshes the privates file in the cache. This was the original implementation as far as I remember. The privates are respected for every class which is ever compiled. It do not see only one library and not only one application. It works on a per-cache folder basis.

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

We do not operate on a per-file level. We use a global dictionary of names which works cache-wide. So this is also not an issue: "I think we will always have problems if we pretend we could do it on a per file level."

It would be sure an issue if the implementation would work that way. Logical.

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

I just downloaded qooxdoo-0.8 SDK. In this release for example it was the case that privates where stored under the globally unique name "privates" directly inside the cache folder.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

As a temporary fix, I did something completely different:

I resolved the application-specific (actually, config.json-specific) privates cache db's, and now there's only one, cache-wide privates db. Now all applications using the same cache will share the same privates db, and all their privates will be registered therein. This should assure a clash-free generation of new private replacements, without any need to process existing cached classes in any way.

The downside is in the size of the compiled apps: As more classes are registered in the privates db, even classes entirely irrelevant for the current build, replacement strings get longer, to ensure uniqueness. We'll have to see how hard this penalty is.

With r20474.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

> I just downloaded qooxdoo-0.8 SDK. In this release for example it was the case that privates where stored under the globally unique name "privates" directly inside the cache folder.

Which was changed when we introduced a system-wide cache, and has now been reverted (see my other comment).

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

Thomas, that was exactly the implementation of 0.8 where a file "privates" was used globally for all applications.

I also don't see a major issue here: For optimal size use separate caches for each application. For optimization build performance use only one cache for multiple applications. Even if you have many many private fields it needs a lot of time to get 4 character long cryptic names. So the size penalty should not be that problematic.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Also cf. bug#2398.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

> I also don't see a major issue here: For optimal size use separate caches for each application. For optimization build performance use only one cache for multiple applications. Even if you have many many private fields it needs a lot of time to get 4 character long cryptic names. So the size penalty should not be that problematic.

Good point.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

assign to watch status in the next couple of days

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

I'm tentatively closing this bug again (see comment from 7/13/09). The last fix, implementing a "third" solution by reverting back to a cache-global privates db, is certainly a step towards eliminating the issue. But again, I'm not entirely sure as the fix undoes a change introduced later than this bug was reported (May 26 vs. May 15). So there might be something still lurking...

Re-open the bug if the problem shows up again.

[qx-bug-importer]

Andreas Ecker (@ecker) wrote:

mass renaming of 0.9 target to 1.0-beta1

[qx-bug-importer]

Martin Wittemann (@wittemann) wrote:

Closed all bugs already shipped with a release.