530222 - Traits::hasCustomConstruct is fragile and error-prone

Status: VERIFIED DUPLICATE of bug 575848

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Steven Johnson]

We use this flag to determine if a class has overridden construct() or not, and if not, we optimize the calls to it.

(1) the default value of this flags is currently false, but it really should be true -- setting it to false (incorrectly) can produce incorrect behavior, while setting it to true (incorrectly) is only a perf penalty

(2) ideally, we should figure out a way to infer this flag, or at a minimum, check in debug builds that it is set correctly (somehow)...

Comment 1

[Steven Johnson]

Attachment: Patch

Addresses the first issue: changes the default to true and then changes the items that should be false. 

I don't have good ideas for the second, but I'm open for suggestions...

Comment 2

[Edwin Smith]

Comment on attachment 413742 [details] [diff] [review]
Patch

this will make all plain user defined types have "hasCustomConstruct=true" which will cause a severe performance penalty, too severe IMO for even a temporary fix

Comment 3

[Edwin Smith]

idea: set a flag in SO::construct, check the flag after calling (virtual) SO::construct, if not set then the method was overridden; then validate that the flag == !hasCustomConstruct

Comment 4

[Steven Johnson]

(In reply to comment #2)
> (From update of attachment 413742 [details] [diff] [review])
> this will make all plain user defined types have "hasCustomConstruct=true"
> which will cause a severe performance penalty, too severe IMO for even a
> temporary fix

True, except that Flash/Air already *do* this -- go look at PlayerAvmCore (or maybe PlayerToplevel), there's a loop that sets this to true for *every* item (and then re-sets to false for selected classes). (The latent bug appeared because AIR wasn't actually doing it for all variants of the builtin pool, but the point is that it wanted -- and *needed* -- to do so in order to get correct behavior). Setting flag by default is merely codifying existing de facto behavior. And to reiterate, current default is simply wrong, and if the wrong answer is ok, then we can make it faster still...

(In reply to comment #3)
> idea: set a flag in SO::construct, check the flag after calling (virtual)
> SO::construct, if not set then the method was overridden; then validate that
> the flag == !hasCustomConstruct

sure, if we can find a place for the flag to live that won't cost us anything. also, is a debug-only, runtime-only verification... better than the status quo, but still susceptible to vagaries of testing.

Comment 5

[Edwin Smith]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 413742 [details] [diff] [review] [details])
> > this will make all plain user defined types have "hasCustomConstruct=true"
> > which will cause a severe performance penalty, too severe IMO for even a
> > temporary fix
> 
> True, except that Flash/Air already *do* this -- go look at PlayerAvmCore (or
> maybe PlayerToplevel), there's a loop that sets this to true for *every* item
> (and then re-sets to false for selected classes). (The latent bug appeared
> because AIR wasn't actually doing it for all variants of the builtin pool, but
> the point is that it wanted -- and *needed* -- to do so in order to get correct
> behavior). Setting flag by default is merely codifying existing de facto
> behavior. And to reiterate, current default is simply wrong, and if the wrong
> answer is ok, then we can make it faster still...

I was under the impression that user code (loaded dynamically) still had hasCustomConstruct=false by default.  not so?

Part of the reason i had this impression is that I recently fixed bugs in the optimization to early bind to constructors, and the fixes mattered :-)  signficant performance improvements occurred.

Comment 6

[Steven Johnson]

> I was under the impression that user code (loaded dynamically) still had
> hasCustomConstruct=false by default.  not so?

It does (and said default is unsafe / incorrect as I pointed out), but may I direct your attention to code in PlayerAvmcCore ctor:

for (uint32_t i = 0; i < class_count; ++i)
{
	m_playerPool->getClassTraits(i)->itraits->hasCustomConstruct = true;
}

Comment 7

[Steven Johnson]

Edwin has just pointed out to me in offline conversations that my patch actually sets hasCustomConstruct=false for all classes, including non-builtin ones, which is a severe performance degradation. The intent of my patch was to default it to true for all *builtin* classes, at which it fails miserably. I'll fix and resubmit.

Comment 8

[Steven Johnson]

Attachment: Patch #2

OK, let's hope this is better: default value of hasCustomConstruct is set to pool->isBuiltin, so all builtin traits default to hasCustomConstruct while nonbuiltins default to hasCustomConstruct=false

Comment 9

[Edwin Smith]

Comment on attachment 414300 [details] [diff] [review]
Patch #2

Shall we keep this open, to follow up with more accurate assignmets for builtin classes that *can* support early bound constructors?

alternatively we should investigate adding support for refactoring the object construction sequence, or adding inline-cache support, or something... we run the risk of just leaving too much performance on the table if the majority of builtin classes require a late bound call for "new".

Comment 10

[Steven Johnson]

(In reply to comment #9)
> (From update of attachment 414300 [details] [diff] [review])
> Shall we keep this open, to follow up with more accurate assignmets for builtin
> classes that *can* support early bound constructors?

absolutely. this patch is a bare-minimum-correctness patch.

Comment 11

[Steven Johnson]

Comment on attachment 414300 [details] [diff] [review]
Patch #2

pushed as changeset:   3220:1c05dd6490d9

Comment 12

[Lars T Hansen]

Attachment #414845 [details] [diff] to bug #531250 attempts to determine something related at run-time but is probably also somewhat brittle, it depends on behavior being invariant but I don't know if the behavior is guaranteed to be invariant.  That is, can a custom constructor change its mind and delegate to the base class's constructor?  Or does this cause so many problems for static typing on the AS3 level that we should not expect that to be an issue?

Comment 13

[Steven Johnson]

(In reply to comment #12)
> can a custom constructor change its mind and delegate to the base class's
> constructor?  

I don't know.

Comment 14

[Lars T Hansen]

(In reply to comment #13)
> (In reply to comment #12)
> > can a custom constructor change its mind and delegate to the base class's
> > constructor?  
> 
> I don't know.

I have introduced a requirement in the patch for bug #531250 that if the overridden createInstance delegates to the base class version then it must /always/ delegate.  Looking through all of the avmglue though I did not find a single instance of this happening - all new versions of createInstance constructed an instance of a subclass of ScriptObject, as you would expect.

Comment 15

[Edwin Smith]

after reviwing comment #9, comment #11, and comment #14, i'm not sure there's anything left to do on this bug.

Comment 16

[Steven Johnson]

See related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=575848

Comment 17

[Edwin Smith]

Work has shifted to bug 575878, closing this one.

Comment 18

[Trevor Baker]

bulk verifying resolved !fixed issues