Updates to the AVM Versioning code to allow more flexibility... the old code pruned unacceptable bindings when an ABC was loaded, including all builtin ABC code. For some embedding clients, this is too late. The gist of the changes is this: - The PoolObject maintains a list of version metadata references. - After the builtin are loaded and we know the SWFversion of the content (these happen in different orders in AIR andFlashPlayer, sadly), we disable methods from non-current versions. - Instead of removing the definitions, they are shunted to a special hiddenVersionNamespace that is accessible to all builtin-code and no user code. - A new flag in the Multiname object indicates that the name occurred in builtin code. - When a reference to a builtin multiname fails the usual way, the special namespace is tried. - TraitsBindings are flushed for any classes that have non-current-version definitions. - (ns,name) to traits/scripts mappings are maintained in various tables in the Pool, Domain, DomainEnv. These are hunted down and the namespaces are adjusted. As things now stand it should be possible for internal code to call methods and properties that are not in the current version, but this ability has not yet been tested. You cannot yet use non-current global definitions (classes, typically) because the init scripts still try to define the original name and not the namespace-mangled name. (This caused a lot of grief until it was realized that it does not affect any user code scenarios... non-current-version classes aren’t initialized because they can’t be referenced. So for now we’re cool as long as builtin code doesn’t try to reference the non-current-version global defs.) Limitations: - It is crude to have a single special namespace rather than a shadow namespace per original source namespace. - It is thuggish to resolve explicit single-ns Multinames to the hiddenVersionNamespace. But so long as it only affects builtin symbol resolution, it should be harmless. - There’s something still messed up with setProp/initProp in the global object but this doesn’t have to work since nothing actually ever causes that to happen until or unless we want to have builtin code call non-current-version classes. Note: this patch doesn't offer any acceptance tests for this feature! A separate bug will be entered to track that.

I don't think we can land this patch until we have said acceptance tests, and probably some documentation on the metadata usage. Will review the code, however.

Comment on attachment 356261 [details] [diff] [review] ±Patch High level problems: - need documentation on requirements & semantics - semantics of a program are undefined after Traits have been modifed. What if the jit re-compiles the code or if the jit is disabled? etc. - there is nothing in the shell that enables new logic to be executed, and no tests to execute the new logic

I would also add that when a patch arrives with comments from the original author containing ".. it is crude .." and ".. thuggish .. " and "..There's something still messed up with ...", it makes it easy to R-

I'm turning this into the tracker bug for this problem.

This patch shows my general approach for versioning support in tamarin. The basic idea is that the version a binding was added in encoded in the URI using a single (non-displayed) unicode character. When a versioned name is encountered the set of "larger" versions is computed and a binding for each is added to the binding table, all pointing to the same value. A necessary optimization is to only add names for the largest version on startup, and then add names for individual versions when ABCs of inactive versions are loaded. (I'm not asking for +review yet, just feedback on the approach.)

Comment on attachment 378433 [details] [diff] [review] preliminary patch for early feedback wrong patch

A couple more points about the patch: - only flash and air package and protected namespaces and the public namespace are versioned - in client code, the uris of those namespaces are marked with a particular version so references only see the bindings of that version - traits in the ABC file with multiple names were added to different versions at different times. all versions are used to compute the set of versions of the corresponding binding.

Not final yet, but good enough for design review

Main comments from conversation with edwsmith: - get host data ahead of time when creating AvmCore and parsing the ABC. No need for callbacks to host to get version data - cache versioned namespaces to minimize construction, interning costs - provide a way to associate each pool object with a specific version. Currently we ask the host for the current version, but it has no way to go deeper than the current Domain - 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

This patch addresses the following items from Comment #11: - get host data ahead of time when creating AvmCore and parsing the ABC. No need for callbacks to host to get version data - provide a way to associate each pool object with a specific version. Currently we ask the host for the current version, but it has no way to go deeper than the current Domain

Comment on attachment 387477 [details] [diff] [review] first candidate A few initial comments from a quick skim... more in-depth comments coming later today or early tomorrow: nit: this style: Namespacep ApiUtils::getCurrentPublic(AvmCore* core, PoolObject* pool) { is at odds with most of the existing code, which is generally Namespacep ApiUtils::getCurrentPublic(AvmCore* core, PoolObject* pool) { the way the string is constructed in ApiUtils::getCurrentPublic seems odd -- why the create/append instead of a single create? there are a few "FIXME optimize me" comments that would be good to expand a bit (ie, why, how?) why is the pool arg to getCurrentPublic a default=null? shouldn't it always be explicitly specified? How much should we worry about the overhead of calling getCurrentPublic()? Might be worth adding NamespaceSet->containsPublic() ? isValidDynamicName() is fairly hot and if there's an optimization for it that would be useful.

Comment on attachment 387477 [details] [diff] [review] first candidate almost a review+, I just have a few questions first... general concerns: ----------------- the fundamental design looks sound but I am a bit concerned that getPublic() and related stuff is introducing work where there was none before. how much have we performance-tested this on big hairy codebases (eg Flex apps)? the various helper functions in ApiUtils are a bit concerning to me; even after having read the overview and the patch, I don't feel 100% comfortable with when and how I need to use them (or not). Would it be possible to add comments to each, helping fill in when you should (and should not) use each? Also: should embedder code ever need to use any of these functions? (hoping the answer is "no") If so, which, and what guidelines for them? To what extent does an embedder need to deal with any of this in his glue code (aside from getting the metadata correct)? possible bugs/glitches: ----------------------- AbcParser.cpp:329 nss->namespaces[0] = ns; do we need a WB() here? nss->namespaces is just an array of naked pointers. (Ditto AbcParser::addNamedTraits() and maybe other places too.) AbcParser.cpp:1554 -- why did the throwVerifyError(kCannotExtendFinalClass) get removed? are we handling the same case elsewhere instead? nits ---- some of the spacing is really whacky -- looks like lots of tabs mixed with spaces. would be nice to unify one way or the other. is ApiUtils::isVersionedNS() hot at all? if so we should rewrite the multiple type checks using a bit-mask...

(In reply to comment #14) > (From update of attachment 387477 [details] [diff] [review]) > A few initial comments from a quick skim... more in-depth comments coming later > today or early tomorrow: > > nit: this style: > > Namespacep ApiUtils::getCurrentPublic(AvmCore* core, PoolObject* pool) { > > is at odds with most of the existing code, which is generally > > Namespacep ApiUtils::getCurrentPublic(AvmCore* core, PoolObject* pool) > { Fixed. > > the way the string is constructed in ApiUtils::getCurrentPublic seems odd -- > why the create/append instead of a single create? Fixed. > > there are a few "FIXME optimize me" comments that would be good to expand a bit > (ie, why, how?) > Fixed. > why is the pool arg to getCurrentPublic a default=null? shouldn't it always be > explicitly specified? Fixed. Replaced with AvmCore::getPublicNamespace(PoolObject* poo) > > How much should we worry about the overhead of calling getCurrentPublic()? A lot, but it pretty efficient now. But still need testing. > Might be worth adding NamespaceSet->containsPublic() ? isValidDynamicName() is > fairly hot and if there's an optimization for it that would be useful. Will keep an eye on it. Jd

(In reply to comment #15) > (From update of attachment 387477 [details] [diff] [review]) > almost a review+, I just have a few questions first... > > general concerns: > ----------------- > the fundamental design looks sound but I am a bit concerned that getPublic() > and related stuff is introducing work where there was none before. how much > have we performance-tested this on big hairy codebases (eg Flex apps)? None yet, but it's work in progress. > > the various helper functions in ApiUtils are a bit concerning to me; even after > having read the overview and the patch, I don't feel 100% comfortable with when > and how I need to use them (or not). Would it be possible to add comments to > each, helping fill in when you should (and should not) use each? I've simplified these somewhat, but there is more work to do to address this comment. These are basically just a bag of internal helpers that shouldn't called except from builtin code. Probably should make some private and friend the builtin classes that need them. Probably not a blocker though. > > Also: should embedder code ever need to use any of these functions? (hoping the > answer is "no") If so, which, and what guidelines for them? To what extent does > an embedder need to deal with any of this in his glue code (aside from getting > the metadata correct)? Shouldn't need to at all. > > possible bugs/glitches: > ----------------------- > > AbcParser.cpp:329 > > nss->namespaces[0] = ns; > > do we need a WB() here? nss->namespaces is just an array of naked pointers. > (Ditto AbcParser::addNamedTraits() and maybe other places too.) Fixed. > > AbcParser.cpp:1554 -- why did the throwVerifyError(kCannotExtendFinalClass) get > removed? are we handling the same case elsewhere instead? Merge munge. Fixed. > > nits > ---- > some of the spacing is really whacky -- looks like lots of tabs mixed with > spaces. would be nice to unify one way or the other. Fixed. Re-tabified changes. BTW, it seems that the code base uses tabs not spaces, contrary to policy. > > is ApiUtils::isVersionedNS() hot at all? if so we should rewrite the multiple > type checks using a bit-mask... Would have fixed, but that would require changing the definition of the NS types. That seems heavy handed until we know there is a problem to be fixed. Jd

Comment on attachment 388488 [details] [diff] [review] second candidate (In reply to comment #18) > I've simplified these somewhat, but there is more work to do to address this > comment. These are basically just a bag of internal helpers that shouldn't > called except from builtin code. Probably should make some private and friend > the builtin classes that need them. Probably not a blocker though. lets at least put comments in there saying what you just said. ArrayClass.cpp: core->getPublicNamespace(NULL) seems quite fishy -- what pool's version should apply? either the caller's pool or the builtin pool should be available here, or a comment is needed why NULL is a special case. ClassClass.cpp, same. probably other call sites too. the array indexing optimization code in CodegenLIR uses the caller's pool, the runtime helper should probably too? (it might be a non-problem if array access helper methods aren't versioned... but at least deserves comments to that effect). adding Lars for one more set of eyes (big change)

(In reply to comment #19) > ArrayClass.cpp: core->getPublicNamespace(NULL) seems quite fishy -- what > pool's version should apply? either the caller's pool or the builtin pool > should be available here, or a comment is needed why NULL is a special case. We want the caller's pool. How do I get it here? CodegenLIR is different because the caller is the code being compiled, but here the caller is only known to the stack, afaikt. getPublicNamespace(NULL) goes an finds it.

(In reply to comment #20) > (In reply to comment #19) > > ArrayClass.cpp: core->getPublicNamespace(NULL) seems quite fishy -- what > > We want the caller's pool. How do I get it here? CodegenLIR is different > because the caller is the code being compiled, but here the caller is only > known to the stack, afaikt. getPublicNamespace(NULL) goes an finds it. ...let's capture this in a comment in the code, then.

Note, "patch" on MacOS reports that the submitted patch is bad, failing on line 2884 (when it starts working on shell/shell_toplevel.as).

(Requires current asc.jar to compile)

Comment on attachment 389500 [details] [diff] [review] second candidate (rebased and unabridged) Approved, though there are numerous style and efficiency concerns as outlined below. I'm not claiming to be grokking everything yet, but the writeup is lucid and the code is largely clean so I'm comfortable trying it out. Overall: Arguably ApiUtils wants its own files? AvmCore is already huge... (not a major objection) Too much temp allocation for my taste makes me suspicious; see comments. I'm guessing I didn't find all problems in this regard, but I also don't know how hot the various functions are. AvmCore.cpp: In AvmCore::newNamespace (first and second signatures), use isEmpty() rather than check for length==0. See comments below for E4XNode.cpp. Note third signature uses comparison with kEmptyString; fix this too. Calls from newNamespace to getBaseURI are not good; the latter constructs a string, we just want to know if it's empty (previous comment). Provide a proper API to test this without allocation. ApiUtils::isPublicNS looks expensive. It calls Namespace::getURI(true), which constructs a new string; the only thing we do with that string is test whether it's empty. If this function is at all hot we risk allocating a lot here for no good reason. Instead provide a specialized API on Namespace to perform the testing that isPublicNS can delegate to. There are some calls to internString that are redundant because the strings are just passed to newNamespace; it takes care of interning. The extra interning incurs a cost in hashing, lookup, comparison. (Also an unconditional allocation at present, see bug #505194.) Speaking of string interning, in setAPIInfo there's the call this->internString(String::createUTF16(...)). This will probably be more efficiently rendered as simply this->internStringUTF16(...) because that avoids the allocation if the string is already interned. Ditto in ApiUtils::getVersionedURI. And if there are two there are probably more. 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 Please fix comment blocks for parseActionBloc, handleActionBlock, handleActionSource since they are just getting more and more out of date. And document the new arguments; these are externally facing APIs. Must document setAPIInfo() et al. Must document ApiUtils. AbcParser.cpp: In AbcParser::parseMetadataInfos if you just (void)a, (void)b unconditionally. 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. E4XNode.cpp: On line 261, the test for empty string is length == 0; the old test compared to core->kEmptyString. The latter cliche is used elsewhere in the code (eg line 302 in the same file). Neither is really appropriate; String::isEmpty() is the right choice, unless there's a chance of a NULL string pointer, in which case a guard against NULL should be used. 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. Namespace.as: As a general rule I think local variables in AS3 code are fully typed; the ones introduced in Namespace::toString should be type-annotated. Namespace.cpp: get_uri() can surely be inlined in Namespace.h NativeFunction.h: Near the end, there's a NULL converted to /*NULL*/ along the way, but the comment should just be removed now. 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. XML.as: As a general rule I think local variables in AS3 code are fully typed; the ones introduced in QName::toString should be type-annotated. XMLObject.cpp: Stray ':' in the comment on line 61 shell/api-versions.cpp Needs MPL note. Needs comments about where it's generated from. This file needs to be a .h file if it's to be included into other files; otherwise tools just get confused. shell/ShellCore.h Please don't stick private data in the middle of the class definition; add it to the private data section at the end. shell/swf.cpp Needs fixing: "// FIXME get this from the SWF"

This is the patch of code being submitted. It responds to many of the comments from Lars. Remaining issues will be tracked with a new bug.

(In reply to comment #24) > (From update of attachment 389500 [details] [diff] [review]) > Overall: > Arguably ApiUtils wants its own files? AvmCore is already huge... (not a major > objection) (tracked in bug 506032) > Too much temp allocation for my taste makes me suspicious; see comments. I'm > guessing I didn't find all problems in this regard, but I also don't know how > hot the various functions are. > AvmCore.cpp: > In AvmCore::newNamespace (first and second signatures), use isEmpty() rather > than check for length==0. See comments below for E4XNode.cpp. Note third > signature uses comparison with kEmptyString; fix this too. Fixed. > Calls from newNamespace to getBaseURI are not good; the latter constructs a > string, we just want to know if it's empty (previous comment). Provide a > proper API to test this without allocation. Fixed. > ApiUtils::isPublicNS looks expensive. It calls Namespace::getURI(true), which > constructs a new string; the only thing we do with that string is test whether > it's empty. If this function is at all hot we risk allocating a lot here for > no good reason. Instead provide a specialized API on Namespace to perform the > testing that isPublicNS can delegate to. Fixed. > There are some calls to internString that are redundant because the strings are > just passed to newNamespace; it takes care of interning. The extra interning > incurs a cost in hashing, lookup, comparison. (Also an unconditional > allocation at present, see bug #505194.) Fixed. > Speaking of string interning, in setAPIInfo there's the call > this->internString(String::createUTF16(...)). This will probably be more > efficiently rendered as simply this->internStringUTF16(...) because that avoids > the allocation if the string is already interned. Ditto in > ApiUtils::getVersionedURI. And if there are two there are probably more. Fixed. > 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. (tracked in bug 506032) > AvmCore.h: > Line 348: "// FIXME memory management" should be fixed (tracked in bug 506032) > Please fix comment blocks for parseActionBloc, handleActionBlock, > handleActionSource since they are just getting more and more out of date. And > document the new arguments; these are externally facing APIs. Fixed (kinda). At least the new 'api' parameter is documented. > Must document setAPIInfo() et al. Fixed. > Must document ApiUtils. Partially fixed. (tracked in bug 506032) > AbcParser.cpp: > In AbcParser::parseMetadataInfos if you just (void)a, (void)b unconditionally. Ignored. > AbcParser::addNamedTraits, second version: unnecessary temp allocation, see > comments as for Traits.cpp below. (tracked in bug 506032) > 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. (tracked in bug 506032) > E4XNode.cpp: > On line 261, the test for empty string is length == 0; the old test compared to > core->kEmptyString. The latter cliche is used elsewhere in the code (eg line > 302 in the same file). Neither is really appropriate; String::isEmpty() is the > right choice, unless there's a chance of a NULL string pointer, in which case a > guard against NULL should be used. Fixed. > 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... (tracked in bug 506032) > Namespace.as: > As a general rule I think local variables in AS3 code are fully typed; the ones > introduced in Namespace::toString should be type-annotated. Fixed. > Namespace.cpp: > get_uri() can surely be inlined in Namespace.h > NativeFunction.h: > Near the end, there's a NULL converted to /*NULL*/ along the way, but the > comment should just be removed now. Fixed. > 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. (tracked in bug 506032) > 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. (tracked in bug 506032) > XML.as: > As a general rule I think local variables in AS3 code are fully typed; the ones > introduced in QName::toString should be type-annotated. Fixed. > XMLObject.cpp: > Stray ':' in the comment on line 61 Fixed. > shell/api-versions.cpp > Needs MPL note. Fixed. > Needs comments about where it's generated from. Fixed. > This file needs to be a .h file if it's to be included into other files; > otherwise tools just get confused. Fixed. > shell/ShellCore.h > Please don't stick private data in the middle of the class definition; add it > to the private data section at the end. Fixed. > shell/swf.cpp > Needs fixing: "// FIXME get this from the SWF" (tracked in bug 506032)

Comment on attachment 390247 [details] [diff] [review] final candidate There's no review requested on this -- does it need another review? If not, is it ready to be pushed?

pushed: changeset - 2212:5d74ffcdc709