Closed Bug 1396613 Opened 2 years ago Closed 2 years ago

Simplify per-class tenuring actions by using the object moved hook

Categories

(Core :: JavaScript: GC, enhancement, P2)

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

(Whiteboard: [qf:p2])

Attachments

(4 files, 4 obsolete files)

In TenuringTracer::moveObjectToTenured there's a big block of if statements that check the type of the object being tenured to work out if we need to do some custom handling when tenuring it.

We should unify this handling with the objectMoved hook that we use when compacting GC moves an object and use this instead.
Whiteboard: [qf]
Patch to change the return type of the object moved class hook to return the size of any additional nursery memory copied in bytes.
Attachment #8907122 - Flags: review?(sphink)
Replace all the special case tenuring logic with use of the class hook.
Attachment #8907127 - Flags: review?(sphink)
Attached patch bug1396613-3-proxy-objects (obsolete) — Splinter Review
Patch to make proxy objects use override the proxy handler objectMoved method rather than using the class hook directly, so we can put common logic in there.

I'll request additional review for this patch (and the first) if you're happy with them.
Attachment #8907128 - Flags: review?(sphink)
Is this safe for FF57? I am comfortable pushing it to 58 unless you think it is very safe.
Flags: needinfo?(jcoppeard)
Priority: -- → P2
Comment on attachment 8907122 [details] [diff] [review]
bug1396613-1-update-hook-return-type

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

::: js/public/Class.h
@@ +722,5 @@
>       * is triggered during construction of the object. This can happen for
>       * global objects for example.
> +     *
> +     * The function should return the number of additional bytes of nursery
> +     * copied to the heap, if any.

It wasn't until I got to the actual code in Marking.cpp that I understood *why* you need to do this. That means it's unclear and should be commented, or I'm dumb. (That's an inclusive or.) Maybe

 * The function should return the difference between nursery bytes used
 * and tenured bytes used, which may be nonzero eg if some
 * nursery-allocated data beyond the actual GC thing is moved into malloced memory.

and then

 * This is used to compute the promotion rate.

if it's true. Is the only reason (other than profiling/telemetry) to compute the promotion rate?

I kind of wonder what happens when we decide to allocate extra space in the nursery (to handle expansion) that we discard when tenuring. That could be an AllocKind change and/or a change in size of  malloc data.

But this is fine.
Attachment #8907122 - Flags: review?(sphink) → review+
Comment on attachment 8907127 [details] [diff] [review]
bug1396613-2-use-hook-for-tenuring

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

I'd like to understand the const situation before marking r+.

::: js/src/gc/Nursery.h
@@ +224,5 @@
> +    void setForwardingPointerWhileTenuring(const void* oldData, void* newData, bool direct) {
> +        if (isInside(oldData)) {
> +            // The caller shouldn't modify oldData, but we're going to store a
> +            // forwarding pointer there.
> +            setForwardingPointer(const_cast<void*>(oldData), newData, direct);

hm, isn't this const_cast UB?

Why is oldData const? I worry that some caller is going to call this function and then read from the pointer, and the compiler will hoist the read above the call.
Attachment #8907127 - Flags: review?(sphink) → feedback+
Comment on attachment 8907128 [details] [diff] [review]
bug1396613-3-proxy-objects

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

This is a great simplification.
Attachment #8907128 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #5)
> It wasn't until I got to the actual code in Marking.cpp that I understood
> *why* you need to do this. That means it's unclear and should be commented

You're right, it is unclear and I need to explain this better in the comments.

> Is the only reason (other than profiling/telemetry) to compute
> the promotion rate?

Yes, it's to compute the promotion rate.

> I kind of wonder what happens when we decide to allocate extra space in the
> nursery (to handle expansion) that we discard when tenuring. That could be
> an AllocKind change and/or a change in size of malloc data.

I'm not sure we really have a concept of AllocKind for nursery objects.  Lots of things can get tenured as a different layout to their nursery form (see JSObject::allocKindForTenure).  But yes, we should take account of any extra space that gets discarded on tenuring.

> hm, isn't this const_cast UB?

I think it's only UB if the original object was delcared as const.

> Why is oldData const? I worry that some caller is going to call this
> function and then read from the pointer, and the compiler will hoist the
> read above the call.

The idea was to stop you modifying the wrong object accidentally.  Conceptually you could say that setting the forwarding pointer is not modifying the object as it does not change its externally visible state.  But maybe we should just remove this const to allow writing forwarding pointers into moved objects.  I'll think some more about this.
(In reply to Naveed Ihsanullah [:naveed] from comment #4)
> Is this safe for FF57? I am comfortable pushing it to 58

I'm not fussed either way, but I think it's safe.  The changes cover a lot of code, but it's mostly plumbing rather than functional changes.
Flags: needinfo?(jcoppeard)
I updated the hook to remove the 'const'.  Other review comments fixed.
Attachment #8907122 - Attachment is obsolete: true
Attachment #8907621 - Flags: review+
And again with the right version of the patch.
Attachment #8907622 - Flags: review+
Attachment #8907621 - Attachment is obsolete: true
Comment on attachment 8907622 [details] [diff] [review]
bug1396613-1-update-hook-return-type v3

Requesting review for browser changes.
Attachment #8907622 - Flags: review?(continuation)
Updated patch with less const_cast.
Attachment #8907127 - Attachment is obsolete: true
Attachment #8907623 - Flags: review?(sphink)
Updated patch.
Attachment #8907128 - Attachment is obsolete: true
Attachment #8907624 - Flags: review+
Comment on attachment 8907624 [details] [diff] [review]
bug1396613-3-proxy-objects v2

Requesting additional review for browser changes.
Attachment #8907624 - Flags: review?(continuation)
Patch to update rust glue for the new signature of the objectMoved hook.
Attachment #8907637 - Flags: review?(nfitzgerald)
Comment on attachment 8907637 [details] [diff] [review]
bug1396613-4-rust-glue

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

Thanks!
Attachment #8907637 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8907623 [details] [diff] [review]
bug1396613-2-use-hook-for-tenuring v2

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

Excellent.
Attachment #8907623 - Flags: review?(sphink) → review+
Whiteboard: [qf] → [qf:p1]
Comment on attachment 8907622 [details] [diff] [review]
bug1396613-1-update-hook-return-type v3

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

r=me for the browser changes
Attachment #8907622 - Flags: review?(continuation) → review+
Comment on attachment 8907624 [details] [diff] [review]
bug1396613-3-proxy-objects v2

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

You should get somebody else to review the CodeGen changes, as they are a little more involved than in the other patch. Maybe Peterv or Boris. I also see an objectMoved thing in CGDOMJSClass, but I guess that doesn't need to be changed because it isn't a proxy?
Attachment #8907624 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #20)
> I also
> see an objectMoved thing in CGDOMJSClass, but I guess that doesn't need to
> be changed because it isn't a proxy?

Yes, this patch only affects proxies.
Comment on attachment 8907624 [details] [diff] [review]
bug1396613-3-proxy-objects v2

Requesting additional review for codegen changes.

The patch changes the way the object moved hook is supplied for proxies from using the class extension hook to overriding the proxy handler method.
Attachment #8907624 - Flags: review?(peterv)
This bug won't be fixed for 57. I am changing it from qf:p1 to qf:p2 for post 57 work.
Whiteboard: [qf:p1] → [qf:p2]
Comment on attachment 8907624 [details] [diff] [review]
bug1396613-3-proxy-objects v2

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

::: dom/bindings/Codegen.py
@@ +1726,5 @@
>  
> +def objectMovedHook(descriptor, hookName, obj, old):
> +    objectMoved = ""
> +    if descriptor.wrapperCache:
> +        objectMoved += "UpdateWrapper(self, self, %s, %s);\n" % (obj, old)

All the callers assert descriptor.wrapperCache (could just move the assert in here?), so I think you can just simplify this to:

    return fill("""
        if (self) {
          UpdateWrapper(self, self, ${obj}, ${old});
        }

        return 0;
        """,
        obj=obj,
        old=old)
Attachment #8907624 - Flags: review?(peterv) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/353300cbbf52
Update the object moved hook to allow it to be called when tenuring nursery objects r=sfink r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/70a52e791eb7
Replace special cases with use of objectMoved hook when tenuring r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/55fc35f2a57d
Make proxy objects override handler's objectMoved method rather than using class hook r=sfink r=mccr8 r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/69536041f010
Update rust glue with new objectMoved hook signature r=fitzgen
You need to log in before you can comment on or make changes to this bug.