Closed
Bug 506032
Opened 15 years ago
Closed 15 years ago
API Versioning needs refinement
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P3)
Tamarin Graveyard
Virtual Machine
Tracking
(Not tracked)
VERIFIED
FIXED
flash10.1
People
(Reporter: jodyer, Assigned: jodyer)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
brbaker
:
review+
|
Details | Diff | Splinter Review |
This bug is for tracking open issues associated with API versioning in AVM
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"
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.
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•15 years ago
|
||
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
Flags: in-testsuite?
Flags: flashplayer-triage+
Flags: flashplayer-qrb?
Comment 5•15 years ago
|
||
ApiUtils::getVersionedURI() doesn't always return an interned URI, but it should.
Attachment #390894 -
Flags: review?(jodyer)
Comment 6•15 years ago
|
||
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).
Attachment #390894 -
Attachment is obsolete: true
Attachment #390898 -
Flags: review?(jodyer)
Attachment #390894 -
Flags: review?(jodyer)
Attachment #390898 -
Flags: review?(jodyer) → review+
Comment 7•15 years ago
|
||
Comment on attachment 390898 [details] [diff] [review]
Namespace URIs must be interned #2
pushed as changeset: 2245:bdf2ac5347e9
Attachment #390898 -
Attachment is obsolete: true
Updated•15 years ago
|
Status: NEW → ASSIGNED
Flags: flashplayer-qrb? → flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.1
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Status: RESOLVED → VERIFIED
Comment 10•15 years ago
|
||
Attachment #408230 -
Flags: review?(brbaker)
Comment 11•15 years ago
|
||
Buildbot patch is dependent on Bug 523823 - patch requires the addition of --enable-api-versioning.
Depends on: 523823
Comment 12•15 years ago
|
||
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
Attachment #408230 -
Flags: review?(brbaker) → review-
Comment 13•15 years ago
|
||
Now using all generic scripts. Updated run-acceptance-generic.sh to allow passing of testdirs.
Attachment #408230 -
Attachment is obsolete: true
Attachment #408426 -
Flags: review?(brbaker)
Updated•15 years ago
|
Attachment #408426 -
Flags: review?(brbaker) → review+
Comment 14•15 years ago
|
||
pushed redux changeset: 2877:80a192bcf79d
Updated•15 years ago
|
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•