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

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

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

Updated

2 years ago
Assignee: nobody → bugs
Assignee

Comment 1

2 years ago
Posted patch forgetSkippable_budget.diff (obsolete) — Splinter Review
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
Assignee

Comment 2

2 years ago
Posted patch v2Splinter Review
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
Attachment #8881756 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8881776 - Flags: review?(continuation)
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.
Attachment #8881776 - Flags: review?(continuation) → review+
Assignee

Comment 4

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

Comment 5

2 years ago
(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.
Assignee

Comment 6

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

Comment 7

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

Comment 8

2 years ago
Posted patch step(5)Splinter Review

Comment 9

2 years ago
Pushed by opettay@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/648731530278
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.