723350 - Simplify gcMallocBytes accounting

Status: RESOLVED INACTIVE

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Attachment: Prevent superfluous last-ditch GCs by improving triggering logic

Bug 679026 added a gcMallocBytes counter to compartments instead of only the runtime. While that allows comp GCs in many situations where previously global GCs occured, it causes an increase in last-ditch GCs.

The reason seems to be the wrong last-ditch trigger described in bug 641025 comment 64.

The attached patch adds a similarly changed last-ditch trigger and makes comp->gcMallocBytes to do so.

I have also cleaned up comp->gcMallocBytes a bit by changing its type to size_t but I can of course move that to another patch if it's too much for this one.

Comment 1

[Brian Hackett [Laid off!]]

Comment on attachment 593652 [details] [diff] [review]
Prevent superfluous last-ditch GCs by improving triggering logic

Looks fine to me, but I think Gregor would be a better reviewer.

Comment 2

[Gregor Wagner [:gwagner]]

Igor, do you know the background for this?
   /*
    * For compatibility treat any value that exceeds PTRDIFF_T_MAX to
    * mean that value.
    */

Comment 3

[Igor Bukanov]

(In reply to Gregor Wagner [:gwagner] from comment #2)
> Igor, do you know the background for this?
>    /*
>     * For compatibility treat any value that exceeds PTRDIFF_T_MAX to
>     * mean that value.
>     */

Once upon a time gcMallocBytes and gcMaxMallocBytes were changed to be ptrdiff_t and gcMallocBytes were descending from the max value towards zero. This allowed to check the maloc allocation via faster gcMallocBytes < 0 in place of gcMallocBytes > gcMaxMallocBytes. However, there was some code that were setting gcMaxMallocBytes to SIZE_T_MAX or similar to disable this checks. With ptrdiff_t that did not work resulting in the above workaround.

Of cause that was premature optimization and I was guilty there. Although it was possible to see its effects in some native code benchmarks, but there was zero effect on anything that could be tested via JS.

Comment 4

[Till Schneidereit [:till]]

Attachment: Prevent superfluous last-ditch GCs by improving triggering logic, v2

After reading Igor's explanation, I wonder why my patch even worked, size_t if course being unsigned and all.

The new patch keeps size_t but accounts for its wrapping around instead of getting to < 0.

A try run should probably be started to verify that working on all platforms.

Comment 5

[Gregor Wagner [:gwagner]]

(In reply to Igor Bukanov from comment #3)
> (In reply to Gregor Wagner [:gwagner] from comment #2)
> > Igor, do you know the background for this?
> >    /*
> >     * For compatibility treat any value that exceeds PTRDIFF_T_MAX to
> >     * mean that value.
> >     */
> 
> Once upon a time gcMallocBytes and gcMaxMallocBytes were changed to be
> ptrdiff_t and gcMallocBytes were descending from the max value towards zero.
> This allowed to check the maloc allocation via faster gcMallocBytes < 0 in
> place of gcMallocBytes > gcMaxMallocBytes. However, there was some code that
> were setting gcMaxMallocBytes to SIZE_T_MAX or similar to disable this
> checks. With ptrdiff_t that did not work resulting in the above workaround.
> 
> Of cause that was premature optimization and I was guilty there. Although it
> was possible to see its effects in some native code benchmarks, but there
> was zero effect on anything that could be tested via JS.

Can we get rid of it? Disabling an internal GC trigger is scary.
This should also only tigger compartment-GCs now.

Comment 6

[Till Schneidereit [:till]]

> Can we get rid of it? Disabling an internal GC trigger is scary.
> This should also only tigger compartment-GCs now.

IIUC, the patch should get rid of it without any side-effects such as losing a GC trigger or any slowdown that would be measurable outside of micro benchmarks as explained by Igor.

Comment 7

[Gregor Wagner [:gwagner]]

I mean for the Runtime counter. It doesn't make sense to disable the check in the runtime but trigger a GC from the compartment.

Comment 8

[Till Schneidereit [:till]]

(In reply to Gregor Wagner [:gwagner] from comment #7)
> I mean for the Runtime counter. It doesn't make sense to disable the check
> in the runtime but trigger a GC from the compartment.

Oh, right, sorry. But maybe in conjunction with bug 716142 it actually would make sense? If not, I could change the patch or add another one to clean up the runtime counter, too.

Comment 9

[Gregor Wagner [:gwagner]]

(In reply to Till Schneidereit [:till] from comment #8)
> (In reply to Gregor Wagner [:gwagner] from comment #7)
> > I mean for the Runtime counter. It doesn't make sense to disable the check
> > in the runtime but trigger a GC from the compartment.
> 
> Oh, right, sorry. But maybe in conjunction with bug 716142 it actually would
> make sense? If not, I could change the patch or add another one to clean up
> the runtime counter, too.

Yeah we have now several other workarounds that don't update the malloc counter. Lets try to remove it from the runtime as well.

Comment 10

[Till Schneidereit [:till]]

(In reply to Gregor Wagner [:gwagner] from comment #9)
> Yeah we have now several other workarounds that don't update the malloc
> counter. Lets try to remove it from the runtime as well.

Ok, I'll do that. I'd rather make it a follow-up bug though, because I think this should be nominated for version 12 to prevent any possible increases in last-ditch GC frequency there.

Do you think keeping the changes to comp->gcMallocBytes in this bug is ok?

Comment 11

[Till Schneidereit [:till]]

Attachment: Removes gcMallocBytes accounting from runtime and simplifies implementation in compartment

As discussed in comment 8 and 9, I have investigated removing the gcMallocBytes accounting from the runtime altogether.

As it turns out, with bug 679026 landed the per-runtime accounting doesn't seem to have any influence on GC whatsoever, anymore: I have run with instrumentation logging any invocations of JSRuntime::onTooMuchMalloc for several days now and haven't encountered any calls to it at all.

For completeness'sake I tried running with and without iGC being enabled, too, so that doesn't seem to make any difference. (As it probably shouldn't, but I wanted to make sure.)

To be certain about the behavior holding across platforms, a try run should probably started (which I still don't have to rights to do).

If that doesn't unearth any problems, I propose removing the accounting from the runtime altogether and simplifying it in JSCompartment as discussed previously. The attached patch does just that.

Comment 12

[Gregor Wagner [:gwagner]]

(In reply to Till Schneidereit [:till] from comment #11)
> Created attachment 620344 [details] [diff] [review]
> Removes gcMallocBytes accounting from runtime and simplifies implementation
> in compartment
> 
> As discussed in comment 8 and 9, I have investigated removing the
> gcMallocBytes accounting from the runtime altogether.
> 
> As it turns out, with bug 679026 landed the per-runtime accounting doesn't
> seem to have any influence on GC whatsoever, anymore: I have run with
> instrumentation logging any invocations of JSRuntime::onTooMuchMalloc for
> several days now and haven't encountered any calls to it at all.
> 


onTooMichMalloc GCs don't happen during normal browsing. They show up when webpages usually do their own memory management with huge arrays. Animations for example are likely to produce more malloced memory than heap allocated.
Bill might be tuning the GC currently so I will forward the review request to him.

Comment 13

[Till Schneidereit [:till]]

> onTooMichMalloc GCs don't happen during normal browsing. They show up when
> webpages usually do their own memory management with huge arrays. Animations
> for example are likely to produce more malloced memory than heap allocated.
> Bill might be tuning the GC currently so I will forward the review request
> to him.

Right, I should have mentioned that I did extensive testing with code that does just that. If there are specific pages known to be good test-cases, I'd be happy to exercise my tests on those, too.

Comment 14

[Gregor Wagner [:gwagner]]

I remember some horrible OOM behavior on low end machines on dromaeo because we didn't correctly update the gcMallocBytes. Embedders also might rely on this.
AFAIK mobile relies on this trigger as well.
So reduce your available memory, open many other pages and run animations and dromaeo again :)

Comment 15

[Till Schneidereit [:till]]

It's not like onTooMuchAlloc GCs aren't triggered at all anymore. My thinking is that the code patterns producing onTooMuchAlloc GCs are ones that lend themselves to compartment GCs very well. Hence, bug 679026 turned almost all or all of those into compartment GCs, and caused the runtime wide counter to become redundant.

In fact, the current factor of 0.9 applied to gcMaxMallocBytes in JSCompartment probably causes animations and similar code to trigger GCs a bit earlier than before.

Comment 16

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

Comment on attachment 620344 [details] [diff] [review]
Removes gcMallocBytes accounting from runtime and simplifies implementation in compartment

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

Thanks, Till. This basically looks good to me. Could you fix the stuff below and upload a new patch?

::: js/src/jscompartment.cpp
@@ +612,5 @@
>  
>  void
>  JSCompartment::resetGCMallocBytes()
>  {
> +    gcMallocBytes = gcMaxMallocBytes - 1;

I'd rather not have the -1 here. See below.

::: js/src/jscompartment.h
@@ +397,3 @@
>          gcMallocBytes = newCount;
> +        /* gcMallocBytes will wrap around and be bigger than gcMaxAllocBytes if a signed value
> +           would be < 0 */

Just change this to a C++ //-style comment.

@@ +397,4 @@
>          gcMallocBytes = newCount;
> +        /* gcMallocBytes will wrap around and be bigger than gcMaxAllocBytes if a signed value
> +           would be < 0 */
> +        if (JS_UNLIKELY(oldCount < gcMaxMallocBytes && newCount > gcMaxMallocBytes))

Can you change this to oldCount <= gcMaxMallocBytes? This I'm assuming we could initialize gcMallocBytes = gcMaxMallocBytes in the reset method.

@@ +397,5 @@
>          gcMallocBytes = newCount;
> +        /* gcMallocBytes will wrap around and be bigger than gcMaxAllocBytes if a signed value
> +           would be < 0 */
> +        if (JS_UNLIKELY(oldCount < gcMaxMallocBytes && newCount > gcMaxMallocBytes))
> +        	onTooMuchMalloc();

This line looks indented too far.

::: js/src/jsgc.cpp
@@ +1668,5 @@
>      JSRuntime *rt = comp->rt;
>      JS_ASSERT(!rt->gcRunning);
>  
> +    bool runGC = rt->gcIncrementalState != NO_INCREMENTAL &&
> +    		(comp->gcBytes > comp->gcTriggerBytes || comp->gcMallocBytes <= 0);

Since gcMallocBytes is unsigned, the |comp->gcMallocBytes <= 0| test seems wrong. To avoid problems like this, please add a method to check if gcMallocBytes has been decremented by more than gcMaxMallocBytes. And then please make gcMallocBytes private again so that people have to use the method.

Comment 17

[Till Schneidereit [:till]]

Attachment: Removes gcMallocBytes accounting from runtime and simplifies implementation in compartment

(In reply to Bill McCloskey (:billm) from comment #16)

Thanks for the review, Bill! I've implemented all of your requests, see specific comments below:

> ::: js/src/jsgc.cpp
> @@ +1668,5 @@
> >      JSRuntime *rt = comp->rt;
> >      JS_ASSERT(!rt->gcRunning);
> >  
> > +    bool runGC = rt->gcIncrementalState != NO_INCREMENTAL &&
> > +    		(comp->gcBytes > comp->gcTriggerBytes || comp->gcMallocBytes <= 0);
> 
> Since gcMallocBytes is unsigned, the |comp->gcMallocBytes <= 0| test seems
> wrong. To avoid problems like this, please add a method to check if
> gcMallocBytes has been decremented by more than gcMaxMallocBytes. And then
> please make gcMallocBytes private again so that people have to use the
> method.

Argh, sorry for that blunder. Goes to show that the trigger has become less important recently: my testing showed it to not make much of a difference anymore.

I wonder, though, whether comp->gcBytes should be made private for the same reason, too? If so, I could do that in a follow-up patch.

Comment 18

[Till Schneidereit [:till]]

Attachment: Removes gcMallocBytes accounting from runtime and simplifies implementation in compartment

grml. I mis-remembered (and didn't re-read until now) Igor's comment 3. In light of its real content, I removed the guards against gcMaxMallocBytes's being set to values > PTRDIFF_MAX.

Other than that, my previous comment applies. Sorry for the bug spam.

Comment 19

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

Comment on attachment 621438 [details] [diff] [review]
Removes gcMallocBytes accounting from runtime and simplifies implementation in compartment

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

Thanks Till. This looks nice.

I think we might want to have a follow-up bug for this. I noticed that the only time JSCompartment::gcMaxMallocBytes changes is when it's set at compartment creation. So if the user wants to change the max malloc bytes (which is possible in Firefox via an about:config preference), they'll only be affecting new compartments. It seems to me that we should also iterate over existing compartments and change their gcMaxMallocBytes. Are you interested in doing this, Till?

I'm not sure if it makes sense for gcBytes to be private. It doesn't have the same problem with overflow, since it's increasing from zero.

::: js/src/jscompartment.h
@@ +290,4 @@
>              onTooMuchMalloc();
>      }
>  
> +    bool isTooMuchMalloc() {

Please mark this as a const method.

Comment 20

[Till Schneidereit [:till]]

Attachment: Removes gcMallocBytes accounting from runtime and simplifies implementation in compartment - now with more const

(In reply to Bill McCloskey (:billm) from comment #19)
> Comment on attachment 621438 [details] [diff] [review]
> Removes gcMallocBytes accounting from runtime and simplifies implementation
> in compartment
> 
> Review of attachment 621438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Till. This looks nice.

Thanks.

> 
> I think we might want to have a follow-up bug for this. I noticed that the
> only time JSCompartment::gcMaxMallocBytes changes is when it's set at
> compartment creation. So if the user wants to change the max malloc bytes
> (which is possible in Firefox via an about:config preference), they'll only
> be affecting new compartments. It seems to me that we should also iterate
> over existing compartments and change their gcMaxMallocBytes. Are you
> interested in doing this, Till?

Right, I saw that, too, but didn't follow up on it. Adding the iteration shouldn't be too hard, so I'll create a bug and ask you for a review if that's ok.

> 
> I'm not sure if it makes sense for gcBytes to be private. It doesn't have
> the same problem with overflow, since it's increasing from zero.

Agreed.

> 
> ::: js/src/jscompartment.h
> @@ +290,4 @@
> >              onTooMuchMalloc();
> >      }
> >  
> > +    bool isTooMuchMalloc() {
> 
> Please mark this as a const method.

Done in the latest patch, carrying the r+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f63829658ff

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2f63829658ff

Comment 23

[Gregor Wagner [:gwagner]]

We should back out this patch. Bug 756549 shows that the global counter is still needed.

Even if we can fix the problem for bug 756549, there might be other situations where we end up in OOM because we removed the global malloc counter.

Comment 24

[Till Schneidereit [:till]]

I agree. Maybe we should only use the global counter if JSRuntime::updateMallocCounter is called without a context, though? I could do that in a follow-up bug.

Comment 25

[Alex Keybl [:akeybl]]

(In reply to Gregor Wagner [:gwagner] from comment #23)
> We should back out this patch. Bug 756549 shows that the global counter is
> still needed.

Agreed - let's back this out asap based upon the fact that multiple people are seeing OOM.

Comment 26

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/08b334d04097

Comment 27

[Ed Morley [:emorley]]

(In reply to Alex Keybl [:akeybl] from comment #25)
> Agreed - let's back this out asap based upon the fact that multiple people
> are seeing OOM.

https://hg.mozilla.org/mozilla-central/rev/08b334d04097

Comment 28

[Till Schneidereit [:till]]

Attachment: Backout of backout

Just a backout of the backout, see next patch for reason

Comment 29

[Till Schneidereit [:till]]

Attachment: Only reset gcMallocBytes of collected compartments, not all

This patch just changes AutoGCSession::~AutoGCSession to only call resetGCMallocBytes of collected compartments instead of all. Applies atop of the previous patch.

Comment 30

[Gregor Wagner [:gwagner]]

Please make it a single patch. No backout of a backout :)

Have you verified that it solves the sync problems?

Comment 31

[Till Schneidereit [:till]]

Comment on attachment 626201 [details] [diff] [review]
Only reset gcMallocBytes of collected compartments, not all

I thought I verified that this patch fixes the regression, but further testing shows that it didn't. Back to the drawing board, then.

Comment 32

[Till Schneidereit [:till]]

Attachment: Fixes and simplifies gcMallocBytes accounting

With the patch from bug 758278, this can be re-landed in a slightly modified form: The new version only invokes resetGCMallocBytes on collected, instead of all compartments.

Comment 33

[Till Schneidereit [:till]]

Attachment: Fixes and simplifies gcMallocBytes accounting, rebased

Rebased

Comment 34

[Gregor Wagner [:gwagner]]

IGC landed again today and we should minimize the GC related changes now. Let's wait until the next Aurora merge with that. This will also allow a full nightly testing period for this. Till, I hope that's ok with you.

Comment 35

[Till Schneidereit [:till]]

Of course that's ok with me. I'm thrilled about iGC re-landing and making the next train and agree about being conservative with GC changes for now.

Comment 36

[Till Schneidereit [:till]]

Review ping. Still applies cleanly to current tip.

Comment 37

[Gregor Wagner [:gwagner]]

Bill, are there any memory regressions left from the IGC landing?

Comment 38

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

It's probably fine to land this at this point. I don't know of any major memory usage regressions.

Comment 39

[Gregor Wagner [:gwagner]]

(In reply to Till Schneidereit [:till] from comment #36)
> Review ping. Still applies cleanly to current tip.

I am currently testing this patch on my machine. I will let you know in a few days.

Comment 40

[Gregor Wagner [:gwagner]]

I am still not convinced that this patch is really what we want.
We still have situations like described in bug 765435 and with
this patch we could end up in more situations where we encounter OOM.

Tracking allocations where we don't have a context or a compartment makes sense for me.

Bill what do you think?

Comment 41

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

I'm kind of on the fence on this one. On the one hand, I'm generally in favor of being aggressive about simplifying/fixing our GC heuristics. If we're not, then the current ones tend to get engrained for no good reason.

However, this patch is a little scary to me because it's not clear that callers of JS_updateMallocCounter are actually using a context with a correct compartment. I was hoping that we could eventually simplify the whole malloc bytes scheme using something like bug 730447, but that doesn't seem to have caught on yet.

Comment 42

[Gregor Wagner [:gwagner]]

(In reply to Bill McCloskey (:billm) from comment #41)
> I'm kind of on the fence on this one. On the one hand, I'm generally in
> favor of being aggressive about simplifying/fixing our GC heuristics. If
> we're not, then the current ones tend to get engrained for no good reason.
> 
> However, this patch is a little scary to me because it's not clear that
> callers of JS_updateMallocCounter are actually using a context with a
> correct compartment. I was hoping that we could eventually simplify the
> whole malloc bytes scheme using something like bug 730447, but that doesn't
> seem to have caught on yet.

Yeah so the idea of this patch is right. We shouldn't have to rely on the global counter. But I don't think we are there yet. When in doubt, I like to be on the safe side.

How about doing this patch in 2 steps: 
First, lets keep the global counter and try to get more information about the cause: Maybe print a huge warning in the errorconsole and the debug output that people should file a bug if the global counter triggers. And also push a version where we assert when it triggers to try.
Second, if we are convinced that we really don't need the global counter any more, we can land this.

Comment 43

[Till Schneidereit [:till]]

I investigated the sources of context-less calls to JS_updateMallocCounter and found that there were only a couple[1] of call sites responsible for the 40k or so calls made during a short-ish browsing session.

I will repeat that experiment and see if I can fix all of those call sites, while getting an impression of how many bytes we're talking about.

After that, I'll use an instrumented build for my normal browsing for a few days and see if anything turns up. Maybe if I actually manage to get all call sites clean, we don't need the global counter anymore. At the very least we'll have an overview of call sites that are harder to fix.

Comment 44

[Gregor Wagner [:gwagner]]

(In reply to Till Schneidereit [:till] from comment #43)
> I investigated the sources of context-less calls to JS_updateMallocCounter
> and found that there were only a couple[1] of call sites responsible for the
> 40k or so calls made during a short-ish browsing session.
> 
> I will repeat that experiment and see if I can fix all of those call sites,
> while getting an impression of how many bytes we're talking about.
> 
> After that, I'll use an instrumented build for my normal browsing for a few
> days and see if anything turns up. Maybe if I actually manage to get all
> call sites clean, we don't need the global counter anymore. At the very
> least we'll have an overview of call sites that are harder to fix.

We have to avoid that there is a theoretical chance of OOM. The number of allocated bytes without a context or compartment might seem small but on mobile that might kill us. We currently have APIs that allow to allocate memory without a context and the only trigger that can prevent OOM is the global counter.

Comment 45

[Till Schneidereit [:till]]

(In reply to Gregor Wagner [:gwagner] from comment #44)
> We have to avoid that there is a theoretical chance of OOM. The number of
> allocated bytes without a context or compartment might seem small but on
> mobile that might kill us. We currently have APIs that allow to allocate
> memory without a context and the only trigger that can prevent OOM is the
> global counter.

Ok, I see what you mean. I'll ponder this some more and see if I come up with something. In the meantime, it seems like there are bigger fish to fry.

Comment 46

[David Mandelin [:dmandelin]]

Comment on attachment 627585 [details] [diff] [review]
Fixes and simplifies gcMallocBytes accounting, rebased

Last comment seems to indicate this is on hold.

Comment 47

[Till Schneidereit [:till]]

I'm not going to work on this anytime soon