413341 - TT: refactor mnpos2hookname and mnpos2dynamicname

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Edwin Smith]

In many cases, the traitsEnv parameter to mnpos2hookname has been guarded as constant, and so its possible to know at trace-time whether the base object is XML or not.  mnpos2hookname() tests for this and branches to a qname constructor or to mnpos2dynamicname().

mnpos2dynamicname() needs to determine if a runtime name contains "public" or not; the common case is a ct-nsset and runtime name, so this fact is usually const even though the name component is not const.

lastly, if an array index is a numeric expression like [i+1] or [r*rows+c], then it will likely be a BoxedDouble and go into _doubleBoxToDynamicName(), where we attempt to convert it to int32 and then uint32, to get a canonicalized number to use as a dynamic name.

in many cases the original expression was computed from ints or uints.  so these double->int conversions amount to overflow checks.  it would be good to find a way to avoid the double->int conversions, in any case.  

the last thing this routine does is convert a double to a string for use as a key.  could we allow doubles to be used directly as HT keys?

Comment 1

[Steven Johnson]

Attachment: Patch

refactored mnpos2hookname and mnpos2dynamicname into other forms to allow us to decide if a compile-name namespace is public or not at trace time, reducing the work we have to do for array accesses. 

Rewrote mn2dyname using more Forth (lots of the old mnpos2dynamicname prim already existed in Forth form). 

Rewrote d2dynamicname in Forth.

Moved mn2name and ?xmlref into e4x/noe4x subfiles since they can be much smarter for the min builds.  

Rewrote Array.dense_push as a forth/native method so we could call it from stack2array. This allows avoiding the huge overhead of setatomprop there. (Array needs massive work, probably a total rewrite, this is just a band-aid.)

Removed a few unused Forth bits.

Added some usually-disabled code for more granular timing in Interpreter.cpp using rdtsc on Intel chips. This allows you to see how much time is being spent in the tracer vs the interp prims vs eot(), etc. and is dumped out along with -Dstats (but is a special compile option)

Fixed a bug with the tracer incorrectly checking for errors in trace generation (should have checked assm->error(), not the unused-and-now-gone Interpreter::error).

Added SSE2 optimization to LIR_fld: if value is 0.0, use XORPD rather than memory.

Tweaked some of the LIR calls that are simply getters or setters to be inline calls after Shark told me it was worth doing.

Fixed latent bug in fc.py: superword trace bodies cannot end in JUMP (the interpreter assumes otherwise) but we didn't check or enforce this. Now we do.

Comment 2

[Steven Johnson]

Note that this patch addresses the first two aspects of this bug, but not the unnecessary int->double->int or double->string conversions. However, it does move the needle across the board (excect for crypto-md5, which actually got a bit slower, but it's already a nightmare due to String.charAt, so I'm not inclined to worry about it just yet...)

                                TT-OLD  TT-NEW
prime4                          193     195
boidsHeadless                   3994    3635
boidshackHeadless               1777    1701
DrawGridHeadless                2527    2397
access-binary-trees             262     253
access-fannkuch                 708     630
access-nbody                    311     306
access-nsieve                   1090    1040
bitops-3bit-bits-in-byte        32      31
bitops-bits-in-byte             94      92
bitops-bitwise-and              504     394
bitops-nsieve-bits              223     186
crypto-md5                      4599    4782
crypto-sha1                     1626    1623
math-cordic                     124     111
math-partial-sums               429     347
math-spectral-norm              117     114
s3d-cube                        1757    1596
s3d-morph                       154     157
s3d-raytrace                    2503    2115
string-fasta                    505     485
AS3Chess_Headless               3532    3956

Comment 3

[Steven Johnson]

re: could we allow doubles to be used directly as HT keys? short answer is, it would be tricky to do properly without revising HT in major ways. It assumes a key is a unique Atom (hence strings must be interned) and has no space for a double-sized key. Not saying it couldn't be done, but it's probably outside the scope of this bug.

re: avoiding the double->int conversions, agreed, but given the limitation mentioned above, I think the best way to do that is to address arithmetic in general (ie, avoiding int->double promotions when possible), which is again outside the scope of this bug (IMHO).

Comment 4

[Edwin Smith]

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

this patch is relative to a52775247362 which is not in tamarin-tracing, so i couldn't apply the patch to look more closely. 

reviewing the diff...

In guard(), isCmp() includes LIR_eq, dont need separate check.

>                    case CI_avmplus_containsPublicNsCT:    // amnpos pool -- b
+

containsPublicNsCT() is PURE-FUNCTION, you shouldn't need this check, should happen automatically.

Is there a problem with a superword ending in jump?  obviously we couldn't continue after the jump but including the jump in the word seems like it should work okay.

Comment 5

[Steven Johnson]

in guard(), I didn't add the check for LIR_eq, just the check for non-null. Agreed that LIR_eq is redundant.

containsPublicNsCT() is PURE-FUNCTION, yep, except that it's really pure solely on the basis of amnpos, pool is irrelevant... but in this case should also be const for a given amnpos. so you're right.

superword ending in jump... hmm, I thought there was, but it looks like the code in the interpreter should handle this after all. I'm going to leave it as-is till I have a chance to go back and verify further.

Comment 6

[Steven Johnson]

pushed as changeset:   297:8531e074ff41

Comment 7

[Chris Peyer]

Closing out all TT: transfer bugs that are resolved fixed.