557538 - custom GC chunk allocation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

To allow for different, platform and embedding dependant strategies of allocating and pooling of the GC chunks we should provide an API that an embedding can use to overwrite the GC chunk allocation implemented in the engine.

Comment 1

[Igor Bukanov]

Attachment: v1

Comment 2

[Igor Bukanov]

Attachment: v2

v1 has tests disabled for debugging

Comment 3

[Mike Moening]

Igor,

I would like to see two more functions instead of callbacks.
Something like:

js_alignmalloc or js_memalign
and
js_alignfree

These functions should have been part of patch for bug 549532
Then when using JS_USE_CUSTOM_ALLOCATOR the caller can override functions that he needs to.

In your case to handle only GC chunk alloc/free. Which would be the only caller of these functions to my knowledge.

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Igor,
> 
> I would like to see two more functions instead of callbacks.

The callbacks are there so code can override the allocation while still using the implementation available in SM for junk pooling and statistics. But for JS_USE_CUSTOM_ALLOCATOR I guess you do not need the implementation. 

The chunk allocation is done via js::AllocGCChunk and js::FreeGCChunk. I can add
an ifdef check not to provide the implementation if JS_USE_CUSTOM_ALLOCATOR is enabled.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 440463 [details] [diff] [review]
v2

The JSAPI stuff looks fine. Just one question:

>+extern JS_PUBLIC_API(size_t)
>+JS_GetGCChunkSizePower(void)
>+{
>+    return GC_CHUNK_SHIFT;
>+}

Why not put this in jsapi.h as a constant? It seems like that might save users some bookkeeping.

In general the test seemed longer than it needed to be. The custom allocator could be as simple as:

    static void *chunk = NULL;

    void *CustomAllocGCChunk(JSRuntime *rt) {
        if (chunk)
            return NULL;  // only give out 1 chunk
        chunk = JS_AllocGCChunk();
        return chunk;
    }

    void CustomFreeGCChunk(JSRuntime *rt, void *p) {
        JS_FreeGCChunk(p);
        chunks = NULL;
    }

In tests.h, destroyRuntime() is fine if you need it; but I didn't see where it was used. If it isn't used, please revert that part. It's easy enough to add later if we ever really do have to have it.

>+    static const size_t MAX_GC_HEEP = size_t(2) * 1024 * 1024;

Nit: MAX_GC_HEAP.

>+    bool ok = evaluate("var max = 0; (function() {"
>+                       "    var array = [];"
>+                       "    for (; ; ++max)"
>+                       "        array.push(max + 0.1);"
>+                       "})();", __FILE__, __LINE__, root.addr());
>+    CHECK(!ok);

Nit: use EXEC or EVAL, like this:

    EXEC("var max = 0; (function() {"
         "    var array = [];"
         "    for (; ; ++max)"
         "        array.push(max + 0.1);"
         "})();");

Then the CHECK is not necessary.

>+    EVAL("(function() {"

EXEC here too.

Then I think you can do away with 'root'.

> #define BEGIN_TEST(testname)                                         \
[...]
>+        static cls_##testname * toTestPtr(void *p) {                 \
>+            return static_cast<cls_##testname *>(p);                 \
>+        }                                                            \

As with destroyRuntime, if you decide you can live without this, please remove it.

If you do need it, how about:

 +        typedef cls_##testname this_test_type;                       \

Then the test can use static_cast<this_test_type *>(p), which seems more self-explanatory.

r=me with whatever changes you think best.

Comment 6

[Jason Orendorff [:jorendorff]]

And I'm sorry for the slow review. I'm backlogged.

Comment 7

[Igor Bukanov]

(In reply to comment #5)
> >+extern JS_PUBLIC_API(size_t)
> >+JS_GetGCChunkSizePower(void)
> >+{
> >+    return GC_CHUNK_SHIFT;
> >+}

I am not sure about this. IMO this will add too much exposure of the internal details into public API header. 

> >+    bool ok = evaluate("var max = 0; (function() {"
> >+                       "    var array = [];"
> >+                       "    for (; ; ++max)"
> >+                       "        array.push(max + 0.1);"
> >+                       "})();", __FILE__, __LINE__, root.addr());
> >+    CHECK(!ok);
> 
> Nit: use EXEC or EVAL, like this:
> 
>     EXEC("var max = 0; (function() {"
>          "    var array = [];"
>          "    for (; ; ++max)"
>          "        array.push(max + 0.1);"
>          "})();");
> 
> Then the CHECK is not necessary.

Noter that I need the reversed check as the code tests that the script should fail with OOM.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Igor, any ETA on getting this fixed up and checked in?  If not, I can apply jorendorff's suggestions above and get it in.  Thanks!

Comment 9

[Igor Bukanov]

Sorry, I have missed this. I will land on Thursday morning European time.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Great, thank you!

Comment 11

[Igor Bukanov]

Yesterday I forgot one important thing. 

I thought initially that something like this patch is a must to have if one wants to implement a shared pool of chunks for jemalloc and GC. But after initial attempts I realized that the proper thing to do would be to allow to link SpiderMonkey against jemalloc on Linux so SM can use functionality available there directly rather than relying on weak malloc/free/realloc symbols been overwritten when linking an executable. But if it would be possible to link against jemalloc than a common pool would be a compile-time option that would not need any runtime API.

Which raises the question: the accounting that can be built on top of this patch would not be available in a generic build and would only be used only for testing, right? If so I would prefer not to add any public API but rather have few FRIEND functions available only when SM compiles with some ifdef.

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

No, we intend to ship this enabled in Firefox releases, to help in-the-field diagnostics of memory consumption issues.  You can see some other measurements in action by visiting about:memory in recent nightlies.

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

That said, FRIEND functions are fine -- the memory reporters in Firefox can be updated to track changes to the friends without much hardship.  Certainly less hardship than getting the initial patch in! :-)

(Apropos which, did this land this morning?  I may be doing bad time-zone math.)

Comment 14

[Igor Bukanov]

I will update the patch to make just friend functions and land it like that.

Comment 15

[Igor Bukanov]

Attachment: v3

The new patch just exposes the necessary functionality via friend API and minimizes the test to use just single chunk as suggested. I also update the error reporting there to avoid useless printouts during a normal ref test run. With this the patch mutated significantly to deserve another review look.

Comment 16

[Andreas Gal :gal]

Comment on attachment 449006 [details] [diff] [review]
v3

Stealing with jorendorff's consent.

>+    typedef void *(* AllocGCChunkOp)(JSRuntime *rt);
>+    typedef void (* FreeGCChunkOp)(JSRuntime *rt, void *chunk);

I think this should be typedef void * (*AllocGCChunkOp), but not sure

>+
>+    AllocGCChunkOp    gcAllocChunkOp;
>+    FreeGCChunkOp     gcFreeChunkOp;
>+
>+    void enableCustomGCChunks(AllocGCChunkOp allocOp, FreeGCChunkOp freeOp) {
>+        JS_ASSERT(allocOp);
>+        JS_ASSERT(freeOp);
>+        JS_ASSERT(state == JSRTS_DOWN);
>+
>+        gcAllocChunkOp = allocOp;
>+        gcFreeChunkOp = freeOp;
>+    }

Wouldn't it be much nicer to do the following:

class CustomChunkAllocator {
    virtual void *alloc(JSRuntime *rt) = 0;
    virtual void free(JSRuntime *rt, void *chunk) = 0;
};

void setCustomChunkAllocator(CustomChunkAllocator *);

And then have a default one. This is a very slow path, and a well predicted branch, so the cost should be 0.

Or maybe even not pure functions and just put in the default one so an implementation can call the original one to fall back on it.

r=me, I much prefer a nice clean C++ interface here, but I can live with the ugly C function pointer stuff too.

Comment 17

[Igor Bukanov]

landed to TM with a C++ class: http://hg.mozilla.org/tracemonkey/rev/9b2f825b2485

Comment 18

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9b2f825b2485