431702 - Move describeType to VM

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Tommy Reilly]

describeType maps traits to XML data structure, at the very least the core VM should have APIs to expose the traits to AS and then describeType can be built in top of that.  Note that the profiler exposes some APIs that are similar to this that we might be able to fold in.

Comment 1

[Steven Johnson]

Attachment: Patch

Add code to enable type introspection to be implemented in TT. This was problematic because some information was no longer retained in non-debug, non-verbose modes, and some was hard to get to. In the end, I provided an API that returns the relevant type information in JSON format via Toplevel::describeType().

There is also a sample implementation of of a Flash-compatible version of describeType(), getQualifiedClassName(), and getQualifiedSuperclassName() in shell/shell.as (System.describeType); Flash uses E4X to expose this information so there is a thin layer of AS3 code to transmogrify the JSON-style data into E4X. (See http://www.adobe.com/livedocs/flex/2/langref/flash/utils/package.html for documentation on the Flash API.) 

The describeType implementation is controlled by AVMPLUS_DESCRIBETYPE; this is currently tied directly to AVMPLUS_MINBUILD but need not be.

A few other unrelated or semi-related changes:

-- resolveNamedTraits_index_fast was misnamed, so I renamed it resolveNamedTraitsEnv_pos_fast

-- removed an unnecessary List<> internal method (_get) that caused GCC to skip some inlining that it could otherwise

-- added a subset of some of Edwin's upcoming Box changes (fromStr, etc) that I wanted to use to implement describeType

-- rearranged some of the bitflags in MethodEnv; the ones that corresponded to MethodKind were no longer needed at all since they come directly from MethodInfo, and the only client of those (IL.cpp) has the MethodInfo necessary at the time.

-- removed chunks of unused code from PrintWriter. Took some other code and made it AVMPLUS_VERBOSE only. Some other stuff became AVMPLUS_PROFILE only.

-- SortedMap<>::get() required the value type to be convertible to 0; added an alternate definition that returns value by reference so that I could use a struct for the value type.

-- fixed a subtle bug in _canDefineMethodSlot that caused some legal get/set overrides to be considered illegal.

-- VMethodInfoNamer_handler took a Thrower, which meant a big chain of callers needed to be able to pass one in. This isn't necessary as the type in question should always be validated before this is called.

-- removed the extendsFlags optimization from TraitsData. This was important in an earlier iteration of our Forth code but is not any more (doesn't make any measurable speed difference in my timings).

Comment 2

[Edwin Smith]

could all the string constants specific to describetype be encapsulated and stuck somewhere else?  lame to have to jam this into avmcore for what ought to be a modular feature.

For that matter, the describeType stuff adds code to Toplevel too, maybe other places.  if its necessary for the code to be in that class, maybe we could still group the implementation code into one file?

there's a lot of C code essentially creating a JSON object graph here.  is there any reasonable way to factor this to use AS3 to do that, and just get into C for access to the TE/ME objects?  (assuming no, or else that would have been done?).  this is a bellweather for how object serialization for AMF will end up looking.

the cleanups look good but i dont think from looking at the code i'll find any obvious bugs... whats the plan for test cases.

Comment 3

[Steven Johnson]

re: string constants, no, they could go elsewhere; it's just the one convenient
place that we keep interned string constants around. (they don't need to be
interned but since we use them a lot for this it doesn't hurt much)

re: toplevel, nope, no compelling reason for it to go there, it was just
convenient, and roughly mimicked the old implementation. I can move the code &
constants into a new file if you like?

re: c code vs AS3, yeah, I wanted to have more of this be AS3, but that would
have required exposing more stuff to AS3 that we don't expose, even internally
(eg SymTable)... I'd like it if this was less C and more AS3 (and it probably
could be with some more work) but I think this is an acceptable implementation
for now and I have a time constraint to get something working soon as Flex uses
this API heavily. (I think the API is flexible enough that we implementation
can be tweaked later.)

re: testcases, I have files internally that need to be cleaned up for testing
purposes. I'll get at least one cleaned up and added to the patch.

Comment 4

[Edwin Smith]

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

i'm sold.  clean up what you can, i'm just in "lets keep the code clean as possible, but no cleaner" mode...

Comment 5

[Steven Johnson]

OK. Want me to move stuff out of Toplevel/AvmCore into a separate file, or submit as-is?

Comment 6

[Steven Johnson]

ended up splitting the code in core and toplevel into a new file (TypeDescriber).

also added testcase in acceptance/bugs.

pushed as changeset:   371:ee5b30dc219c

Comment 7

[Brent Baker]

The testcase bugs/bug431702.as is failing on the THUMB builds of windows mobile. It passes on the ARM builds, but fails THUMB jit and interp.

Comment 8

[Steven Johnson]

Oops. It will probably be a few days before I can investigate this. Can we mark it "expected fail" for the moment (unless someone else has time to debug sooner)?

Comment 9

[Brent Baker]

Marked as expected failure via patch 379:d9bab4b9afcd

Comment 10

[Brent Baker]

Looks like patch 419:6984d3bf9e71 has caused the testcase bugs/bug431702.as to start failing on 

- ALL Release_Debugger builds 
- Linux Release builds

Comment 11

[Steven Johnson]

Oops -- describeType was relying on SymTable being sorted alphanumerically, but this change now sorts by the pointer value of the interned string, so it's no longer order-dependent. Should be simple to fix with an explicit sort, and it's really an issue with predicabiility (output is still "correct" but may vary in ordering from run to run). Assigning to self, but may be a few days before I get to it.

Comment 12

[Brent Baker]

Pushed a patch to expect this testcase to fail on releasedebugger and release on linux (429:7f1389d188e0)

Comment 13

[Steven Johnson]

it should probably be expect-to-fail everywhere, as the failure is intermittent, depending on the whims of memory allocation. (it fails frequently on my local mac)

Comment 14

[Steven Johnson]

Attachment: Patch

Patch to correct the injection -- now we explicitly sort the fields that might be in unpredictable orders. (Note, I don't think Flash ever did this, but it's cheap and makes testing much easier.) In doing so I discovered (and fixed) a bug in Array.sortOn() that crept in during the AS3 rewrite... oops.

Note that the testfile itself was modified to add some additional testing with namespaces.

Comment 15

[Edwin Smith]

should we do the sort in the test case?

Comment 16

[Steven Johnson]

Yeah, I suppose we could, it's just trivial to do ahead of time.

Comment 17

[Steven Johnson]

Attachment: Patch

Revised patch: per Edwin's suggestion, put the sorting into the testcase instead of the describeType implementation. (Left in the fix to Array.sortOn, though.)

Comment 18

[Edwin Smith]

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

if anyone complains about the builtin code being slower due to sortOn we can take that out later.

stable test cases are a good thing.

Comment 19

[Steven Johnson]

pushed as changeset:   443:91f3ca5f4e64