506032 - API Versioning needs refinement

Status: VERIFIED FIXED

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

Description

[Jeff Dyer]

This bug is for tracking open issues associated with API versioning in AVM

Comment 1

[Jeff Dyer]

Carried over from bug 472920:

###

> (From update of attachment 389500 [details] [diff] [review] [details])
> Overall:
> Arguably ApiUtils wants its own files?  AvmCore is already huge... (not a major
> objection)

###

> And more, if namespaces are to be interned frequently then constructing temps
> as the code does may not be as good as providing a new internNamespace function
> that avoids the construction.  If these code paths are hot, that is.

###

> AvmCore.h:
> Line 348: "// FIXME memory management" should be fixed

###

> Must document ApiUtils.

###

> AbcParser::addNamedTraits, second version: unnecessary temp allocation, see
> comments as for Traits.cpp below.

###

> AbcParser::addNamedScript, temporary namespace sets containing one element are
> constructed in callers of this function, but could that be avoided?  See
> comments as for Traits.cpp below.

###

> Also on line 261, kEmptyString's atom is used to compare to the prefix to test
> for an empty prefix presumably; I doubt this is correct as well.  I see
> kEmptyString's atom is being used to construct namespaces elsewhere, but is
> this always the case?
> Note, there /can/ be multiple empty strings because it is possible to construct
> empty strings of different widths.  

Not related to API versioning, but...

###

> Traits.cpp:
> It seems unnecessary to create a new NamespaceSet in the second version of
> addVersionedBindings (line 661) if you can avoid it - it depends on how hot you
> expect this method to be.  I realize this may require a second version of
> getCompatibleNamespaces.  But we should strive to reduce the amount of temp
> allocation we do.

###

> Ditto in buildBindings, though here it seems harder to avoid given the
> complexity it would introduce.  However, since the main consumer of the nss
> seems to be addVersionedBindings it's really the same problem.

###

> shell/swf.cpp
> Needs fixing: "// FIXME get this from the SWF"

Comment 2

[Jeff Dyer]

And some more comments from Steven on bug 472920:

> 
> How much should we worry about the overhead of calling getCurrentPublic() (now findPublicNamespace())?

###

> Might be worth adding NamespaceSet->containsPublic() ? isValidDynamicName() is
> fairly hot and if there's an optimization for it that would be useful.

Comment 3

[Jeff Dyer]

And unanswered concerns from Ed in bug 472920:

> Main comments from conversation with edwsmith:
> - cache versioned namespaces to minimize construction, interning costs
> - test for the increase in number of bindings for a large flex app
> - we need to grok the consequences of having multiname traits in library code

Comment 4

[Brent Baker]

QRB: target for current release, this is getting close to wrapping up
QE: marked as in-testuite:? we will need to have some testcases provided for testing the version within the shell

Comment 5

[Steven Johnson]

Attachment: Namespace URIs must be interned

ApiUtils::getVersionedURI() doesn't always return an interned URI, but it should.

Comment 6

[Steven Johnson]

Attachment: Namespace URIs must be interned #2

Better fix: existing usage implies that newNamespace() should do interning if necessary, so I ensured that all 3 forms of that did the interning in all cases, rather than having ApiUtils::getVersionedURI() do it.

Also replaced a couple of createUTF16/append pairs with append16 for marking (avoids a temporary String).

Comment 7

[Steven Johnson]

Comment on attachment 390898 [details] [diff] [review]
Namespace URIs must be interned #2

pushed as changeset:   2245:bdf2ac5347e9

Comment 8

[Jeff Dyer]

Capturing review comments from Lars (copied from Bug 508076):

* re apiBit, the following has more compact source code (as would a simple
loop), but it's unclear if it's smaller in code size or executes fewer
instructions than the current version (it depends on whether the api string
tends to have low bits set or high bits set).  The last three computations
could be replaced by a simple table lookup, though, and that might win - if we
care.

uint32_t apiBit(int32_t api)
{
    uint32_t tmp = (uint32_t)api;
    uint32_t res = 0;
    if ((tmp & 65535) == 0) { res += 16; tmp >>= 16; }
    if ((tmp & 255) == 0)   { res += 8;  tmp >>= 8; }
    if ((tmp & 15) == 0)    { res += 4;  tmp >>= 4; }
    if ((tmp & 3) == 0)     { res += 2;  tmp >>= 2; }
    if ((tmp & 1) == 0)     { res += 1; }
    if (api != (1 << res)) 
    {
    AvmAssert(!"Only one bit should be set");
    res = 0;
    }
    return res;
}

* If isVersionedURI() is hot we can probably do better than the binary string
search by using some sort of trie or discriminator tree (or jitting the
discriminator code :-)  Even now it seems that we should skip the search if
uri->length() is smaller than the length of the shortest URI or longer than the
length of the longest URI - if that happens often enough to care about.

Comment 9

[Jeff Dyer]

Most of the notes captured here are obsolete, the others are niceties and are not required. So for the sake of bug hygiene I'm closing this bug. If you disagree, let's open a new one that is more focused.

Comment 10

[Chris Peyer]

Attachment: Add api-versioning building and testing to buildbot.

Comment 11

[Chris Peyer]

Buildbot patch is dependent on Bug 523823 - patch requires the addition of --enable-api-versioning.

Comment 12

[Brent Baker]

Comment on attachment 408230 [details] [diff] [review]
Add api-versioning building and testing to buildbot.

master.cfg:
- would rather see the compilation use the compile-generic script instead of adding another boilder plate script (see linux-deep for usage)
- test_api_versioning step is using the wrong script, should be ../all/run-acceptance-release-api.sh

run-acceptance-release-api.sh:
- should attempt to download the shell if it does not exist, in the event that the shell is compiled on a different slave (same pattern as all other scripts)
- should use --notimecheck on runtests.py and also update the echo to display what is being run

Comment 13

[Chris Peyer]

Attachment: Updated buildbot api-versioning patch

Now using all generic scripts.  Updated run-acceptance-generic.sh to allow passing of testdirs.

Comment 14

[Chris Peyer]

pushed redux changeset:   2877:80a192bcf79d