TM: avoid JSArena as temp buffer in snapshot()

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 final-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

9 years ago
Blocks: 502832
(Assignee)

Updated

9 years ago
Blocks: 517909
(Assignee)

Comment 1

9 years ago
Blocking a beta blocker.
Flags: blocking1.9.2?
(Assignee)

Comment 2

9 years ago
Created attachment 405918 [details] [diff] [review]
patch
Assignee: general → gal
Attachment #405918 - Flags: review?(lw)

Comment 3

9 years ago
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
(Assignee)

Comment 5

9 years ago
http://hg.mozilla.org/tracemonkey/rev/1a747dd43904
Whiteboard: fixed-in-tracemonkey
(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
(Assignee)

Comment 8

9 years ago
#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

9 years ago
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.
(Assignee)

Comment 10

9 years ago
This change reduces our total allocations from 325MB to 285MB on SS (!).
Unblocking beta on this.  P2.
Priority: P1 → P2
(Assignee)

Comment 12

9 years ago
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

9 years ago
http://hg.mozilla.org/mozilla-central/rev/1a747dd43904
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Flags: blocking1.9.2+ → blocking1.9.2-

Updated

9 years ago
Flags: blocking1.9.2- → blocking1.9.2+
You need to log in before you can comment on or make changes to this bug.