1376614 - Pass budget to forgetSkippable and return early when budget has been used

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

In some cases forgetSkippables are too slow. I think we could make it more incremental.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: forgetSkippable_budget.diff

This seems to trigger the overbudget every now and then when running speedometer

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a8ed0c53a14c8c530d4b66eeaebd12237ab8673

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

I think I want always some budget.

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/4647333853181099895b35d7b99f79cda3ae0a97
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=4647333853181099895b35d7b99f79cda3ae0a97

Comment 3

[Andrew McCreight [:mccr8]]

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

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

r=me with these fixes

::: xpcom/base/nsCycleCollector.cpp
@@ +1073,5 @@
>      auto iterFromLastEntry = mEntries.IterFromLast();
>      for (; !iter.Done(); iter.Next()) {
>        nsPurpleBufferEntry& e = iter.Get();
>        if (e.mObject) {
> +        if (!aVisitor.Visit(*this, &e)) {

It is a little ugly to require these checks here, but the alternative would be to always check a budget in here, and then pass in unlimitedBudget for the other cases, which isn't great either. So I guess this is okay.

@@ +2780,5 @@
> +    if (mBudget.isOverBudget()) {
> +      return false;
> +    }
> +
> +    mBudget.step();

Should this be stepped by a larger value? With 1, we only actually check the time every 1000 things in the purple buffer.

@@ +2804,4 @@
>    }
>  
>  private:
> +  js::SliceBudget mBudget;

This should be a reference. There's state in the SliceBudget that gets modified when we check the budget.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

We could pass budget as value or reference, doesn't really matter in the current setup.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> ::: xpcom/base/nsCycleCollector.cpp
> @@ +1073,5 @@
> >      auto iterFromLastEntry = mEntries.IterFromLast();
> >      for (; !iter.Done(); iter.Next()) {
> >        nsPurpleBufferEntry& e = iter.Get();
> >        if (e.mObject) {
> > +        if (!aVisitor.Visit(*this, &e)) {
> 
> It is a little ugly to require these checks here, but the alternative would
> be to always check a budget in here, and then pass in unlimitedBudget for
> the other cases, which isn't great either. So I guess this is okay.
Yeah. I just took the simplest approach


> @@ +2780,5 @@
> > +    if (mBudget.isOverBudget()) {
> > +      return false;
> > +    }
> > +
> > +    mBudget.step();
> 
> Should this be stepped by a larger value? With 1, we only actually check the
> time every 1000 things in the purple buffer.
Well that is what MarkRoots does. Though that is too high. Perhaps I could try 5.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I can check how many other entries were removed from purple buffer (because of CanSkip) and pass also that number to step

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Er, no I can't. I don't have that information.
I guess I'll just use random number 5

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: step(5)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/648731530278
Pass budget to forgetSkippable and return early when budget has been used, r=mccr8

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/648731530278