549532 - Allow custom memory allocator use in spidermonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Moening]

Enhancement to allow use of a user defined custom memory allocator with the JS engine. This bug will hold the enhancement and any bug fixes needed to support this feature.

The feature will be turned off/on with #define JS_USE_CUSTOM_ALLOCATOR.

1st bug:  JS_ARENA_DESTROY macro uses free() instead of js_free()

Comment 1

[Mike Moening]

2nd bug:  SystemAllocPolicy uses free() malloc() and realloc() directly instead of js_free() js_malloc(), js_realloc().

Comment 2

[Mike Moening]

3rd bug:  jdhash.cpp DefaultAllocTable() and DefaultAllocEntry() have mismatched pair of malloc and js_free().  Changing all to use js_malloc().

Comment 3

[Mike Moening]

Attachment: custom allocator patch

Removed static global Oracle and make part of TLS inside TraceMonitor.
Static global Oracle was allocating memory before a custom memory manager could be instantiated (at module load time).

Fixed up serveral malloc/free pairs to use js_malloc/js_free.

Added JS_USE_CUSTOM_ALLOCATOR define to enable use of user supplied custom memory manager via new header.

Comment 4

[Mike Moening]

Attachment: patch #2

Simplified patch.  Less oracle passing around...

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 431553 [details] [diff] [review]
patch #2

Patch looks great, per-TM oracles make much more sense.

Flushing it when we flush the JIT cache will probably cause speculation to happen more often (since we flush more often than we GC, I think... globals and all). I want to test this in the browser before landing, to see how SS is affected. I'll do this tomorrow.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/04023ea0fb08

SS perf looked stable.

Comment 8

[Mike Moening]

Just a follow up on this since I saw Franck's duplicate bug.

This patch left out only two additional memory allocation spots which call VirtualAlloc() (in windows at least..others for linux etc).

The JIT tracer code that uses this needs executable memory which most memory managers don't provide. So that must be left alone for now. Executable memory can ony be provided by VirtualAlloc() on WIN32.

The 2nd, which we should fix, involves allocating GC chunks using VirtualAlloc() again.  The code was expecting the returned memory to be aligned.  I didn't have time to sort this one out. The alignment may not be necessary...It may impact performance too.

I didn't have time to test the impact and was hitting ASSERT's. 
This is food for another bug...

Franck you game?

Comment 9

[Brendan Eich [:brendan]]

As pointed out on IRC, the VirtualAlloc (or mmmap) calls are for good reason. Alignment is not optional. Please don't malloc-ify blindly.

/be

Comment 10

[Mike Moening]

We need to add js_memalign() function for this. 
Also since HAS_POSIX_MEMALIGN is no longer used we probably need a corresponding
js_freemealign() or something to that affect.

Comment 11

[Brendan Eich [:brendan]]

The OSes differ and we've sorted through the options by benchmarking. Adding js_ veneer is not helpful. Please let this code alone -- it's not relevant to memory pressure feedback for the GC -- it *is* the GC's underlying allocator.

/be

Comment 12

[Mike Moening]

Adding these functions certainly isn't for feedback.

Those VirtualAlloc() calls made for GC chunks are a major cause of OS memory thrashing.  There are frequenct calls to VirtualAlloc() VirtualFree(). Way too frequent.

We really need to be able to put in a custom memory manager under those too.
Adding js_memalign will not hurt performance and gives an opportunity for custom allocator use. Anything to cut down on kernel transitions is a win.

So long so we don't impact browser perfs I can't see why this would be bad.

Comment 13

[Brendan Eich [:brendan]]

Mike: not this bug. Custom memory allocator != stop VirtualAlloc/Free thrashing. You don't add custom hookability to fix an intrinsic problem, that just piles the bovine stuff higher :-/.

See bug 508707 and especially bug 541140 (which is going in soon, I think) first. I think you just want bug 541140 fixed.

/be

Comment 14

[Mike Moening]

Brendan,

I didn't say it was a "bug".
You are correct about intrinsic problems. Yes I know that.
These kind of intrinsic problems take a long time to fix or even get looked at.
Since they may not affect the browser path. Which gets priority.

However, if you are using a custom allocator you need to ALWAYS use the custom allocator.  At least as much as you can.  Not just sometimes.

If the custom allocator overcomes some intrinsic problems...then...GREAT!
Looks like the patch in bug 508707 eliminates the use VirtualAlloc in NewGCChunk() which would take care of the issue by default.
So that's fine by me. If.....that patch ever lands.

Bug 541140 1st comment by Greg Wagner hits the nail on the head!
That's exactly what I'm saying.  Too much trashing. 
Giveth back memory and taketh away again.. Very silly.

I suspect in a threaded world its actually MUCH worse that Greg even knows.
Yes lets fix that bug... Definately.

But for now I'll take a custom allocator every time.

If embedders are using a custom allocator they should expect consistency from the engine...regardless of intrinsic problems.

Mike

Comment 15

[Brendan Eich [:brendan]]

I think we should fix bug 508707 too but even bug 541140 would be enough, and that has the big momentum now. We shouldn't just add another layer to allow you to hack some equivalent fix (or something less fix-y that may blow up on you). Let's just fix that bug.

/be

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

backed out: http://hg.mozilla.org/tracemonkey/rev/f54ca5383515

mac build failures:

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1268307120.1268307481.23297.gz&fulltext=1

Comment 17

[Mike Moening]

Attachment: Patch #3

Removed some #ifdef JS_TRACER code from js_cntxt.h which was causing NON JS_TRACER builds to fail on MAC OS X builds.

Comment 18

[Igor Bukanov]

For the record: JS_NewString(JSContext *cx, char *bytes, size_t nbytes) takes an ownership of the passed byte array and release it later using js_free. If an embedding uses a custom allocator and uses that API it must ensure that the bytes array is allocated using the custom allocator and not using, for example, strdup.

Comment 19

[Mike Moening]

Igor,

It seems to me this pattern is broken even without a custom allocator.
JS_NewString() should not make assumptions about how (char* bytes) is allocated.

The propensity for memory mismatch errors with new/free or malloc/delete are huge in this case.  Not only that but possible double free.

I doubt the API will change...
Thankfully the online documentation is good on this.
Thanks for information either way.

Mike

Comment 20

[Brendan Eich [:brendan]]

MikeM: JS_NewString (should have been JS_NewStringMallocHandoff or some such ugly name) is ~15 years old. It's not like you "shoulding" all over it makes its rocky face flinch in the least! :-/ It has rules, they need to be followed.

Better APIs to lead developers away from it are good, but a handoff of malloc'ed chars storage API is still a common use-case. With C++ it can be a lot safer and more convenient, I think.

/be

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/04023ea0fb08

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 431982 [details] [diff] [review]
Patch #3

>-JS_REQUIRES_STACK static JS_INLINE void
>-MarkSlotUndemotable(JSContext* cx, LinkableFragment* f, unsigned slot)
>+JS_REQUIRES_STACK void
>+TraceRecorder::MarkSlotUndemotable(JSContext* cx, LinkableFragment* f, unsigned slot)
> {

Now that this is part of TraceRecorder, it doesn't need a |cx| anymore.

>+TraceRecorder::MarkSlotUndemotable(JSContext* cx, LinkableFragment* f, unsigned slot, const void* pc)

Ditto.

>-static JS_REQUIRES_STACK unsigned
>-FindUndemotesInTypemaps(JSContext* cx, const TypeMap& typeMap, LinkableFragment* f,
>+JS_REQUIRES_STACK unsigned
>+TraceRecorder::FindUndemotesInTypemaps(JSContext* cx, const TypeMap& typeMap, LinkableFragment* f,

Here as well.

> JS_REQUIRES_STACK void
>-CaptureStackTypes(JSContext* cx, unsigned callDepth, TraceType* typeMap)
>+TraceRecorder::CaptureStackTypes(JSContext* cx, unsigned callDepth, TraceType* typeMap)

Here, too.

r=me with those nits.

Comment 23

[Brendan Eich [:brendan]]

s/MarkSlotUndemotable/markSlotUndemotable/g too now that it's a method.

/be

Comment 24

[Mike Moening]

Attachment: Final with nits picked

Removed cx parameter from some methods and made them lowercase per style recommendations.

Comment 25

[Mike Moening]

Attachment: Final re-based to tip

Comment 26

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

I'm so used to putting "[JAEGER]" in my commits that I accidentally did it here. Sorry!

Comment 27

[Robert Sayre]

this is causing a crash in jsctypes tests:

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1271126267.1271127848.28658.gz&fulltext=1

stack trace available.

Comment 28

[Mike Moening]

Robert,

I'm tinderbox tests were failing before this patch was pushed from what I can see.

Mike M.

Comment 29

[Mike Moening]

Attachment: Final patch - part 1

Breaking that patch into 2 parts for landing. Maybe 3 before I'm done.

Comment 30

[Mike Moening]

Attachment: Final patch - part 2

Comment 31

[Robert Sayre]

(In reply to comment #29)
> Created an attachment (id=439334) [details]
> Final patch - part 1

pushed the first bit:
http://hg.mozilla.org/tracemonkey/rev/f72b7ada0948

Comment 32

[Mike Moening]

Well part 1 landing had no ill effects.

When can we land part2?

It shouldn't take 1.5 months to land a patch like this.

Comment 33

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/4a28bd424400

Comment 34

[[e-mail]]

Comment on attachment 439336 [details] [diff] [review]
Final patch - part 2

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

Comment 35

[[e-mail]]

Backed out due to bustage.

Comment 36

[Mike Moening]

Attachment: Final patch - part 2 rebased

Updated for patch rotting.

Comment 37

[[e-mail]]

http://hg.mozilla.org/tracemonkey/rev/72104246b23b

Pushed for real.