729353 - GC: Implement the Generational WriteBuffer

Status: RESOLVED DUPLICATE of bug 764962

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0: A trivial generational buffer, no compacting.

This comes in two patches... it appears bugzilla will only let me attach one at a time.  The attached patch is a trivial write buffer.  When it gets full, we reset the write pointer to the beginning: no compaction scan, no hashing.  The second patch (which I will attach shortly) adds the writes into the WriteBuffer in all of the postWriteBarriers that we care about.

The following benchmark was done on an opt x86 build, with no js args (interpreter only), on a stably clocked machine.  Its sole purpose is to discover the overhead of adding the WriteBuffer write to all writes in the interpreter.

Before:
       crypto: 0.02 sys 79.00 user 1:19.29 wall 51200 kb
    deltablue: 0.07 sys 82.75 user 1:23.19 wall 127828 kb
 earley-boyer: 0.27 sys 96.70 user 1:37.74 wall 107088 kb
     raytrace: 0.06 sys 33.16 user 0:33.36 wall 132728 kb
       regexp: 0.18 sys 4.29 user 0:04.49 wall 48200 kb
     richards: 0.01 sys 68.57 user 1:08.83 wall 14772 kb
        splay: 0.08 sys 4.27 user 0:04.37 wall 252064 kb

After:
       crypto: 0.02 sys 78.56 user 1:18.84 wall 51604 kb
    deltablue: 0.06 sys 80.36 user 1:20.74 wall 128044 kb
 earley-boyer: 0.04 sys 94.43 user 1:34.83 wall 107640 kb
     raytrace: 0.19 sys 47.91 user 0:48.66 wall 132992 kb
       regexp: 0.30 sys 7.62 user 0:08.09 wall 48640 kb
     richards: 0.09 sys 69.38 user 1:09.94 wall 14976 kb
        splay: 0.09 sys 4.27 user 0:04.39 wall 254488 kb

Comment 1

[Terrence Cole [:terrence]]

Attachment: v0: use the post barriers

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 599400 [details] [diff] [review]
v0: A trivial generational buffer, no compacting.

Review of attachment 599400 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, although we should do benchmarks to decide how complex the allocation need to be.

I'm worried that the platform-dependent aspects of catching writes to the write-protected pages will be really complex. I'm guessing that Breakpad already installs a signal handler to deal with segfaults, so we need to make sure to cooperate with it. That sounds really icky.

::: js/src/gc/WriteBuffer.h
@@ +20,5 @@
> +const size_t Alignment = 64 * 1024;
> +const size_t IdBufferSize = 1 * Alignment;
> +const size_t ValueBufferSize = 1 * Alignment;
> +const size_t ObjectBufferSize = 4 * Alignment;
> +const size_t SlotBufferSize = 4 * Alignment;

Is there any need to declare these in a header file?

@@ +23,5 @@
> +const size_t ObjectBufferSize = 4 * Alignment;
> +const size_t SlotBufferSize = 4 * Alignment;
> +
> +template<typename T>
> +class EdgeBuffer

Can this type be a member of WriteBuffer rather than having to use the detail namespace?

@@ +62,5 @@
> +{
> +    const JSObject *object;
> +    uint32_t offset;
> +
> +    SlotRef(const JSObject *obj, uint32_t off) : object(obj), offset(off) {}

No const on JSObject, please.

@@ +81,5 @@
> +    friend class detail::EdgeBuffer<Value *>;
> +    friend class detail::EdgeBuffer<JSObject **>;
> +    friend class detail::EdgeBuffer<SlotRef>;
> +
> +    JSRuntime *rt_;

This should probably be runtime_ (or just runtime).

@@ +84,5 @@
> +
> +    JSRuntime *rt_;
> +    void *nursery_;
> +
> +    void *buffer_;

Eventually, you probably want to write a destructor to clean up this memory.

::: js/src/jsgc.cpp
@@ -913,5 @@
>  JSBool
>  js_InitGC(JSRuntime *rt, uint32_t maxbytes)
>  {
> -    if (!InitMemorySubsystem())
> -        return false;

Is this supposed to be here?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 599401 [details] [diff] [review]
v0: use the post barriers

Review of attachment 599401 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobjinlines.h
@@ +2096,5 @@
>  JSObject::privateWriteBarrierPost(void **old)
>  {
> +#ifdef JSGC_GENERATIONAL
> +    if (uintptr_t(*old) < 32)
> +        return;

I think maybe this can be just a NULL test.

::: js/src/vm/String-inl.h
@@ +64,5 @@
>  inline void
>  JSString::writeBarrierPost(JSString *str, void *addr)
>  {
> +#ifdef JSGC_GENERATIONAL
> +    if (uintptr_t(str) < 32)

I think this can also be just a NULL check.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

By the way, the numbers in comment 0 look suspicious. Regexp and splay are way lower than the rest.

Comment 5

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #4)
> By the way, the numbers in comment 0 look suspicious. Regexp and splay are
> way lower than the rest.

Hurm... they run much faster than the others in the jits as well, though, don't they?  The larger weirdness I noticed is the fact that all but 2 of the tests got faster _with_ the barriers in place.

I ran both sets several times: these are both from a single, representative run.  I should have averaged the numbers, but I'm not exactly sure which number we care about between wall time and system time.  I think most of the close ones (I think splay is in this group?) are just noise, but the regexp slowdown is definitely significant.  I will rebase and try to find out what's going on exactly.

(In reply to Bill McCloskey (:billm) from comment #2)
> This looks fine to me, although we should do benchmarks to decide how
> complex the allocation need to be.
> 
> I'm worried that the platform-dependent aspects of catching writes to the
> write-protected pages will be really complex. I'm guessing that Breakpad
> already installs a signal handler to deal with segfaults, so we need to make
> sure to cooperate with it. That sounds really icky.

I'm not too worried about it.  My plan is to make the buffer size such that it will either work fine or fail immediately.
 
> ::: js/src/gc/WriteBuffer.h
> @@ +20,5 @@
> > +const size_t Alignment = 64 * 1024;
> > +const size_t IdBufferSize = 1 * Alignment;
> > +const size_t ValueBufferSize = 1 * Alignment;
> > +const size_t ObjectBufferSize = 4 * Alignment;
> > +const size_t SlotBufferSize = 4 * Alignment;
> 
> Is there any need to declare these in a header file?

My thinking is that it might be good to be able to grow these dynamically.  I expect that a browser runtime is going to want a larger write buffer than a random js worker.
 
> @@ +23,5 @@
> Can this type be a member of WriteBuffer rather than having to use the
> detail namespace?

I'll see if I can just forward-declare them and leave the implementation in the cpp.
 
> ::: js/src/jsgc.cpp
> @@ -913,5 @@
> >  JSBool
> >  js_InitGC(JSRuntime *rt, uint32_t maxbytes)
> >  {
> > -    if (!InitMemorySubsystem())
> > -        return false;
> 
> Is this supposed to be here?

Nope: merge bustage.  I had this repo pointed at the repo that added SlotsRef, but before that it was pointed at the repo that added gc/Memory.  Looks like part of that patch didn't make the swap correctly.

Last time this sort of thing happened, qpopping everything and pulling from mozilla-inbound still left me with the outstanding change, even though the pull succeeded and reported success.  Looks like I'm going to have to just make a new repo base on the patch again.  *sigh*

Comment 6

[Terrence Cole [:terrence]]

And this has been checked in as part of the larger bug 764962.