547713 - MathUtils::convertDoubleToString makes big allocation it might not ever use

Status: RESOLVED WONTFIX

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

Description

[Tommy Reilly]

I noticed that the heap was often expanding converting numbers to a string, or for many other simple math operations. It turns out that that inside MathUtils::convertDoubleToString, we allocate and free a D2A class every time we do a conversion. This would not be so bad except that the D2A class includes 4 BigInteger values. As a result we allocate and free a 2K buffer every time we we do a conversion. This size of buffer can easily cause the heap to expand. That makes for a pretty expensive conversion. The actual conversion I traced used a bigInteger on the stack and would very rarely make use of the values in the class.

--- gavin


Can we make the 2k allocation on demand instead?

Comment 1

[Steven Johnson]

Gavin -- looking at the code, it appears we only allocate and use D2A when we call with a mode other than  DTOSTR_NORMAL... which, as far as I can tell, is only when Number.toFixed/toPrecision/toExponential are called from AS3. Are these the cases you are seeing?

Comment 2

[Steven Johnson]

Oops -- nevermind; the DTOSTR_NORMAL case applies only when the input value is integral.

Comment 3

[Rick Reitmaier]

If we're going to touch this routine can we also look at making it more amenable to PrintWriter?  See bug 562480 comment #14.

What would be ideal (for that scenario) is to push the output to PrinterWriter&,f for the cases where a string is needed use a StringBuffer, although judging from the description this might be too heavy-weight allocation wise.

Comment 4

[Jason Boyer]

Confirmed that the space for the BigIntegers is allocated with every D2A.  In addition, there are several BigIntegers on the stack, unnecessarily increasing the stack. Based on chatting with treilly, I am proposing to make three changes:

1. Use a new class GCBigInteger, which will be allocated only when needed.
2. Use stack allocation for the newly small D2A class.
3. Abstract out the behavior where we currently branch based on bFastEstimateOK. We would then check the condition for bFastEstimateOK, and delegate to a D2ADouble or D2ABigInteger class, or may D2AFast and Slow.

Any comments welcome.

Comment 5

[Lars T Hansen]

A more traditional approach would perhaps be to just create a factory for D2A buffers in AvmCore or in some new D2ABufferHolder class (which would end up being referenced from AvmCore anyway), and just alloc and free buffers through that, instead of going for a GCBigInteger, probably.  The buffer holder could have a ceiling on the number of free buffers, maybe, if we think that it might hold onto too many, but my impression is not that that's a concern.  (Some data would be useful if we suspect that it is.)

With that in mind it's not obvious that we'd care about whether we allocate those buffers all the time or not, because we wouldn't be actually allocating them - after the first warmup we'd be reusing existing buffers.  So that would mean no changes needed to the formatting code, which (in the spirit of PACMAN) in turn would reduce risk and churn.

Comment 6

[Jason Boyer]

Attachment: Possible patch, to see if this is what Lars had in mind.

OK, this works in the test environment, but do we need anything fancier, such as the QCache objects that are used elsewhere in AvmCore, or locks for thread safety?

Comment 7

[Lars T Hansen]

Comment on attachment 559295 [details] [diff] [review]
Possible patch, to see if this is what Lars had in mind.

I figured you would cache the buffers, not the d2a itself, but OK - this is a pretty minimal patch, hard to argue.  (My only concern was that there's less state to get confused about in the buffers during reset - but at this point it's not much to get worked up over.)  A plus of your solution is that D2A instances created in the old way by non-VM code will not impact the one the VM caches for its own use.

The other issue then is to verify somehow that it's right to cache one, and not more than one.

There should be no thread safety concerns, anyone calling obtainD2A is running on the main thread I should think.  Worth verifying that MathUtils is not being abused by the Flash Player probably, stranger things have happened.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.