521880 - TM: avoid JSArena as temp buffer in snapshot()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Andreas Gal :gal]

JSArena is evil and does malloc()/free() every time, which is horribly slow. Use a growing vector instead.

Comment 1

[Andreas Gal :gal]

Blocking a beta blocker.

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 405918 [details] [diff] [review]
patch

Righteous.

Comment 4

[Damon Sicore (:damons)]

Marking these blocking so we can get them landed asap.

Comment 5

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/1a747dd43904

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #0)
> JSArena is evil and does malloc()/free() every time, which is horribly slow.
> Use a growing vector instead.

This comment is innaccurate. What "every time" means is when you go from an empty arena pool to non-empty and back. But that's still not the whole story, since the "growing vector" wins not because it can grow but because it has a fixed "inline-allocated" hunk of memory to use, if it can.

If arenas are evil they should be removed, but of course you'd reinvent them and either add an inline-allocated layer on top, or have them hang onto memory and tend to bloat when they should return it to the malloc heap. But perhaps time-based caching could be used.

In any event, file a bug that deals with the trade-offs accurately and fairly. Don't bring the arena-defamation-league truth squad down again!

/be

Comment 7

[Brendan Eich [:brendan]]

I do mean "file a bug on time-based caching of arenas". We already have that for the stack arena-pool, see jsgc.cpp's FREE_OLD_ARENAS and look for "timestamp" in jsinterp.cpp. Perhaps this should be generalized and made all shiny with C++ and stuff. That would be progress.

jsarena.[ch] are over 14 years old, based on that cited LCC paper. They certainly could use some love. No hate, though.

/be

Comment 8

[Andreas Gal :gal]

#0 is a bit incomplete/inaccurate. Using the arena in this context is bad/evil, because it tends to hit the malloc/free path all the time. What wins here is not so much the inline-allocated buffer (thats a win on top of this), but we resize internally and carry forward the tempTypeMap while we compile and then destroy it when we are done.

I do want to rewrite the arena code. igor has a patch in his queue that de-macro-ifies the code a bit, and starting from there we should rework the arena code a bit. I like the timestamp idea. I also would like to try power-of-2-until-limit-is-reached growth for arena chunks instead of fixed size growth. Over the last 14 years available memory has increased a lot, so we can probably tolerate that and save a bunch of malloc/free pairs (especially free seems to be expensive, so maybe just timestamping and re-using fix this).

Comment 9

[Luke Wagner [:luke]]

Another idea David and I discussed was postponing arena-shrinkage until the outermost js_Interpret invocation exits.  This has the advantage that it avoids the "shrink yet?" check on the arena free path.

Comment 10

[Andreas Gal :gal]

This change reduces our total allocations from 325MB to 285MB on SS (!).

Comment 11

[Damon Sicore (:damons)]

Unblocking beta on this.  P2.

Comment 12

[Andreas Gal :gal]

I landed a new version of this to fix the ts regression the original patch causes. This seems to give us a 1% txul improvement without ts regression. dvander, can you please review the landed patch.

http://hg.mozilla.org/tracemonkey/rev/f722ab349f94

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1a747dd43904

Comment 14

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3ba29f919575