JSArena is evil and does malloc()/free() every time, which is horribly slow. Use a growing vector instead.
Blocking a beta blocker.
Created attachment 405918 [details] [diff] [review] patch
Assignee: general → gal
Attachment #405918 - Flags: review?(lw)
Comment on attachment 405918 [details] [diff] [review] patch Righteous.
Attachment #405918 - Flags: review?(lw) → review+
Marking these blocking so we can get them landed asap.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
(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
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
#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).
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.
This change reduces our total allocations from 325MB to 285MB on SS (!).
Unblocking beta on this. P2.
Priority: P1 → P2
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
status1.9.2: --- → final-fixed
You need to log in before you can comment on or make changes to this bug.