Jon Coppeard (:jonco) Reporter
	Description • 7 years ago

If we see that the nursery is getting full we can call a callback in the browser to dispatch an idle runnable which performs a nursery collection.

	Chia-Hung Duan Assignee
	Comment 3 • 7 years ago

I think this callback can do more rather than only for nursery collection. IMO, it should be a runtime state monitor, we determine what to do during idle time by the runtime state and the time restriction (i.e. before the end of idle time). I think this idea is a little far from this bug's work.

Several things to discuss:

1. It's not easy to determine the threshold of triggering a idle nursery collection, because it depends on the length of idle time and now I have no idea about the required time for each type of GC or each stage of GC. So I temporary use "NurseryFreeThresholdBytesForInspectation = NurseryFreeThresholdBytes / 2" as a threshold.

2. As mentioned above, the runnable may take the expiration idle time as argument so that the runtime can determine what scale of GC or something should be taken. Because at this point we don't have such mechanism so I didn't bring that function in this patch. Maybe another bug for this?

3. At the beginning of working on this bug, I think we should let the user to dispatch the idle runnable by themselves.
But there are too many entry points for executing JS... Ah, this is just a comment about the implementation.

	Chia-Hung Duan Assignee
	Comment 8 • 7 years ago

Really sorry for the list of redundant patches... I'm trying to set 8-line context.

	Adam Gashlin (he/him) [:agashlin] (ex-moco)
	Comment 9 • 7 years ago

Here is a microbenchmark I was using when exploring this earlier, it creates and loses objects in the nursery inside a requestAnimationFrame (to give an opportunity for the idle callback to run). With your current patch I am seeing a 1.7-2.0% improvement in frame times. Median times went from 1.72 (75% 1.88) to 1.69 (75% 1.84), consistent across 4 runs each. This was on a Macbook Pro, I'll get some numbers from the quantum reference hardware soon.

Note that this takes around 3 minutes to run, |count| can probably be cut down a lot and still be accurate instead of doing 10,000 frames. The numbers reported are median (50th percentile), 75th percentile, and 95th percentile. I was expecting the median to stay the same while only 75% or 95% decreased, it's a nice surprise that they all seem to have gone down.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 7 years ago

Worth to test also how this affects to Speedometer 2.

	Adam Gashlin (he/him) [:agashlin] (ex-moco)
	Comment 11 • 7 years ago

I haven't noticed anything significant (given the error bars) on http://browserbench.org/Speedometer2.0/ , except that with cduan's patch the error seems consistently smaller, which may be due to doing GC in idle times.

with patch: 84.3 +/- 1.17, 85.8 +/- 1.17, 84.3 +/- 1.3
without: 84.6 +/- 3.9, 85.9 +/- 2.0, 82.7 +/- 2.8

(Again this is with a Macbook Pro, on Windows 10, reference hw numbers later today)

	Adam Gashlin (he/him) [:agashlin] (ex-moco)
	Comment 12 • 7 years ago

I'm seeing roughly the opposite effect on reference hardware, without the patch is median 1.20 (1.33 75%), with is 1.24 (1.37 75%), 3% longer with.

No significant difference on Speedometer 2.0, 73.7 +/- 1.5 with, 74.3 +/- 0.93 without.

	Chia-Hung Duan Assignee
	Comment 14 • 7 years ago

Hi Adam, according to your microbenchmark, I found a bug which may cause the performance regression, thanks.

I update a new one. I'll try the microbenchmark tomorrow and share the numbers.

	Jon Coppeard (:jonco) Reporter
	Comment 15 • 7 years ago

Comment on attachment 8902640 [details] [diff] [review]
Support idle runnable for nursery collection

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

::: js/src/gc/Allocator.cpp
@@ +59,5 @@
> +                ArenaCellSet::NurseryFreeThresholdBytesForInspectation) &&
> +                !rt->hasPendingInspectation()) {
> +              rt->callInspectationCallback();
> +              rt->setPendingInspectation(true);
> +            }

This doesn't trigger the callback for objects allocated by the JIT, which I would expect to happen a lot for hot code.

It may be possible to combine this threshold test with the end of nursery test for JIT code but if not I'd rather not have an extra test done for every allocation.

I wonder whether it's possible to check the nursery state every time the browser goes idle if this is not too frequent.

::: js/src/jsapi.cpp
@@ +1388,5 @@
>  JS_PUBLIC_API(void)
> +JS_InspectRuntimeState(JSRuntime* rt) {
> +    // We may need a system to inspect the whole runtime state and take corresponding actions.
> +    // Now we only support nursery collection. Check js::Allocate for more details.
> +    rt->gc.minorGC(JS::gcreason::EVICT_NURSERY);

It's possible a minor GC already happened before our idle callback.  It would be a good idea to check the free space here to see if we still need to do this.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 7 years ago

How fast is the nursery state check? 
If fast, we could perhaps check the state even after each task, http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/js/xpconnect/src/XPCJSContext.cpp#1068 (that is for main thread only)
and if collection is probably needed, then dispatch idle runnable?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 7 years ago

http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/xpcom/base/CycleCollectedJSContext.cpp#348 gets called on main thread and workers.

	Jon Coppeard (:jonco) Reporter
	Comment 18 • 7 years ago

(In reply to Olli Pettay [:smaug] from comment #16)
It's a handful of arithmetic operations and a compare so it should be quick enough.

	Chia-Hung Duan Assignee
	Comment 20 • 7 years ago

(In reply to Jon Coppeard (:jonco) from comment #15)
> Comment on attachment 8902640 [details] [diff] [review]
> Support idle runnable for nursery collection
> 
> Review of attachment 8902640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/Allocator.cpp
> @@ +59,5 @@
> > +                ArenaCellSet::NurseryFreeThresholdBytesForInspectation) &&
> > +                !rt->hasPendingInspectation()) {
> > +              rt->callInspectationCallback();
> > +              rt->setPendingInspectation(true);
> > +            }
> 
> This doesn't trigger the callback for objects allocated by the JIT, which I
> would expect to happen a lot for hot code.

Fixed. I thought this is the entry point for all allocation, thanks.

> It may be possible to combine this threshold test with the end of nursery
> test for JIT code but if not I'd rather not have an extra test done for
> every allocation.
> 
> I wonder whether it's possible to check the nursery state every time the
> browser goes idle if this is not too frequent.
> 
> ::: js/src/jsapi.cpp
> @@ +1388,5 @@
> >  JS_PUBLIC_API(void)
> > +JS_InspectRuntimeState(JSRuntime* rt) {
> > +    // We may need a system to inspect the whole runtime state and take corresponding actions.
> > +    // Now we only support nursery collection. Check js::Allocate for more details.
> > +    rt->gc.minorGC(JS::gcreason::EVICT_NURSERY);
> 
> It's possible a minor GC already happened before our idle callback.  It
> would be a good idea to check the free space here to see if we still need to
> do this.

I agree with your points above. Maybe we have three steps to accomplish it.

1. Bring up a runtime monitor. As mentioned before, the best way is to let the runtime monitor makes the decision.
2. Check the runtime state when the browser goes idle and pass the length of idle time for JS_InspectRuntimeState.
3. Merge with this nursery collection patch. 

These three items can be independent, i.e. we can merge the 3. first then 1. and 2.
What do you think?

	Chia-Hung Duan Assignee
	Comment 21 • 7 years ago

(In reply to Adam Gashlin [:agashlin] from comment #12)
> I'm seeing roughly the opposite effect on reference hardware, without the
> patch is median 1.20 (1.33 75%), with is 1.24 (1.37 75%), 3% longer with.
> 
> No significant difference on Speedometer 2.0, 73.7 +/- 1.5 with, 74.3 +/-
> 0.93 without.

I found the microbenchmark keeps draining out the nursery and during that test, it triggers a lot of GCs, so there is almost no idle time for testing the impact of this patch. Maybe we should find another way to evaluate this.

	Adam Gashlin (he/him) [:agashlin] (ex-moco)
	Comment 22 • 7 years ago

(In reply to Chia-Hung Duan from comment #21)
> I found the microbenchmark keeps draining out the nursery and during that
> test, it triggers a lot of GCs, so there is almost no idle time for testing
> the impact of this patch.

I'm sorry if this isn't an appropriate test, if you get a chance could you help me understand where I've gone wrong?

I'm not sure what you mean by "draining out the nursery", I had understood that the nursery only fills up until a minor GC. We want to trigger a lot of GCs, but when looking at the devtools profiler it seems like these are mostly minor, I was actually concerned that there weren't enough minor GCs to really show the difference. But I didn't want it to leak, as that would cause more major GC activity, so it keeps a constant amount of memory alive (half of the allocation).

The intention was to simulate something that made a number of allocations inside of a requestAnimationFrame handler as an animating script might do; this provides a strict deadline for how long we want the handler to take (ideally minor GC wouldn't happen while the handler is running as that could cause us to miss the frame), while also providing idle time outside of that handler (where the minor GC could run without interfering, or so I thought?). What I was expecting was Minor GC time moved out from the RAF handler to some other idle time, and thus out of the time measured for the frame.

I should have commented like this in the microbenchmark itself...

	Chia-Hung Duan Assignee
	Comment 23 • 7 years ago

(In reply to Adam Gashlin [:agashlin] from comment #22)
> (In reply to Chia-Hung Duan from comment #21)
> > I found the microbenchmark keeps draining out the nursery and during that
> > test, it triggers a lot of GCs, so there is almost no idle time for testing
> > the impact of this patch.
> 
> I'm sorry if this isn't an appropriate test, if you get a chance could you
> help me understand where I've gone wrong?
> 
> I'm not sure what you mean by "draining out the nursery", I had understood
> that the nursery only fills up until a minor GC. We want to trigger a lot of
> GCs, but when looking at the devtools profiler it seems like these are
> mostly minor, I was actually concerned that there weren't enough minor GCs
> to really show the difference. But I didn't want it to leak, as that would
> cause more major GC activity, so it keeps a constant amount of memory alive
> (half of the allocation).
> 
> The intention was to simulate something that made a number of allocations
> inside of a requestAnimationFrame handler as an animating script might do;
> this provides a strict deadline for how long we want the handler to take
> (ideally minor GC wouldn't happen while the handler is running as that could
> cause us to miss the frame), while also providing idle time outside of that
> handler (where the minor GC could run without interfering, or so I
> thought?). What I was expecting was Minor GC time moved out from the RAF
> handler to some other idle time, and thus out of the time measured for the
> frame.
> 
> I should have commented like this in the microbenchmark itself...

I apologize for my negligence so that I thought the benchmark might don't work.

Last day before I upload the new patch, I ran this benchmark and logged the idle time GC.
I didn't see any minorGC is triggered by idle time callback. Thus I guess it may request too
many objects during each requestAnimationFrame so that the GCs are triggered by allocation failed
(i.e.,  we should reduce the iteration count of the push loop).
I should double check that, but I didn't...

The scenario is ok; I don't mean to judge the flow of benchmark. I do see the idle time GC event
today(I'm not sure what's happened yesterday).

Everything is clear in the microbenchmark, Thanks!

	Adam Gashlin (he/him) [:agashlin] (ex-moco)
	Comment 24 • 7 years ago

(In reply to Chia-Hung Duan from comment #23)
> I apologize for my negligence so that I thought the benchmark might don't work.

No worries, I'm not sure it is appropriate either, I'm new to all things GC.

	Chia-Hung Duan Assignee
	Comment 26 • 7 years ago

This patch uses periodic idle runnable to do nursery collection(i.e. without allocation overhead)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 27 • 7 years ago

Comment on attachment 8904975 [details] [diff] [review]
Support periodic idle runnable for nursery collection

This doesn't look too good. We end up running mRuntimeInspectator all the time, like every 16ms. That causes possibly lots of wakeups.

	Chia-Hung Duan Assignee
	Comment 28 • 7 years ago

(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8904975 [details] [diff] [review]
> Support periodic idle runnable for nursery collection
> 
> This doesn't look too good. We end up running mRuntimeInspectator all the
> time, like every 16ms. That causes possibly lots of wakeups.

Because it is woken up at idle time and in most of cases it does nothing, so I guess it's ok. Maybe I neglect something.
If this approach is acceptable, we can find a proper time interval.

As I know, for a garbage collector, expect to trigger by allocation failed, there are two common ways to run a background collector or issue a GC request.
1. Periodically check the heap state.
2. Check the heap state after each allocation.

So I implemented these two methods but I'm not sure which one is better for JSengine or to be more precisely, for Firefox.
I think we can discuss this :)

	Jon Coppeard (:jonco) Reporter
	Comment 29 • 7 years ago

(In reply to Chia-Hung Duan from comment #28)
I think we should check the heap state in XPCJSContext::AfterProcessTask() as suggested by Olli in comment 16.  It looks like this already happens for the CC.

	Jon Coppeard (:jonco) Reporter
	Comment 30 • 7 years ago

Comment on attachment 8904975 [details] [diff] [review]
Support periodic idle runnable for nursery collection

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

Thanks for working on this.  This is along the right lines but we need to split the runtime inspection into two pieces: first we should check whether a GC is required, and if so we should schedule an idle task.  Secondly, in the idle task we should perform the GC.  We can do the check in XPCJSContext::AfterProcessTask() so we won't have a periodic timer firing.

::: js/src/jsapi.cpp
@@ +1391,5 @@
> +    // the proper actions based on the length of idle time.
> +
> +    // Now we only support nursery collection at idle time.
> +    if (rt->gc.nursery().needIdleTimeCollection() ||
> +        rt->gc.nursery().minorGCRequested()) {

If minorGCRequested() is true, we should trigger the GC with the reason code returned by minorGCTriggerReason().  For GC triggered by needIdleTimeCollection() we should add a new reason code so we can see how often this happens.  Reason codes can be added in js/public/GCAPI.h.

::: js/src/jsapi.h
@@ +1729,5 @@
>  /*
>   * Garbage collector API.
>   */
>  extern JS_PUBLIC_API(void)
> +JS_InspectRuntimeState(JSRuntime* rt, mozilla::TimeStamp/* unused idle time currently*/);

This needs a less generic name.  Maybe something like 'IsIdleGCTaskNeeded' or 'ShouldTriggerIdleGCTask' to check and then something like 'RunIdleGC' to do the GC.

(Also, please put new API functions in the JS namespace rather than using the JS_ prefix).

	Chia-Hung Duan Assignee
	Comment 31 • 7 years ago

(In reply to Jon Coppeard (:jonco) from comment #29)
> (In reply to Chia-Hung Duan from comment #28)
> I think we should check the heap state in XPCJSContext::AfterProcessTask()
> as suggested by Olli in comment 16.  It looks like this already happens for
> the CC.

Yes, I had noticed that. But I think that is not good to do the idle runnable things.

Dispatch idle runnable in XPCJSContext::AfterProcessTask() is almost the same as the periodic idle runnable approach.
The difference is that periodic idle runnable should be enqueued periodically, but XPCJSContext::AfterProcessTask() implicitly take the piece of time between two tasks.

Except for the things that HTML processing model specified we should do, I think we shouldn't do extra things(although most of the checks are cheap). That's the reason why I didn't do it in XPCJSContext::AfterProcessTask()

As your comment, I will work on "check whether a GC is required" first, it is bug 1397237.

Thanks for the review!

	Jean Gong :jgong
	Comment 32 • 7 years ago

This won't be address in 57; changing qf:p1 to qf:p3 for post 57 work.

	Jon Coppeard (:jonco) Reporter
	Comment 34 • 7 years ago

Comment on attachment 8923321 [details] [diff] [review]
Support idle runnable for nursery collection

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

This is great.

Flagging Olli for review of cycle collector changes.

::: js/src/gc/Nursery.h
@@ +290,5 @@
>      /* The amount of space in the mapped nursery available to allocations. */
>      static const size_t NurseryChunkUsableSize = gc::ChunkSize - gc::ChunkTrailerSize;
>  
> +    /* Attemp to run a minor GC in the idle time if the free space falls below this threshold. */
> +    static constexpr size_t kIdleTimeCollectionThreshold = 512 * 1024;  // 512 KB

This threshold is probably too large.  Most of the time the browser will have a 1MB nursery, and this could effectively halve this size.

I'd suggest making this 128KB.  We can always increase it later.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 35 • 7 years ago

Comment on attachment 8923321 [details] [diff] [review]
Support idle runnable for nursery collection

 
>+void CycleCollectedJSContext::CheckJSRuntimeState() {
In Mozilla coding style[1] (which JS engine doesn't follow yet),  { goes to its own line with
class and method declarations.

>+  // Get JSRuntime from CycleCollectedJSRuntime.
>+  JSRuntime* js_runtime = Runtime()->Runtime();
jsRuntime

>+
>+  class IdleTimeGCTaskRunnable : public mozilla::IdleRunnable {
{ to its own line

>+  public:
>+    explicit IdleTimeGCTaskRunnable(JSRuntime* rt) : rt_(rt) { }
Arguments should be named aName, so
JSRuntime* aRt


>+  public:
>+    NS_IMETHOD Run() override {
{ to its own line

>+      JS::RunIdleTimeGCTask(rt_);
>+      return NS_OK;
>+    }
>+
>+    nsresult Cancel() override {
ditto

>+      return NS_OK;
>+    }
>+
>+  private:
>+    JSRuntime* rt_;
JS_Runtime* mRt;
What guarantees this never points to a deleted object? Is it guaranteed this code never runs during shutdown after runtime has been deleted.
But I don't actually see the need for this. Run() method could always just access the current runtime.
Something like
CycleCollectedJSRuntime* ccrt = CycleCollectedJSRuntime::Get();
if (ccrt && ccrt->Runtime()) {
  JS::RunIdleTimeGCTask(ccrt->Runtime());
}


>+  };
>+
>+  if (JS::IsIdleGCTaskNeeded(js_runtime)) {
>+    nsCOMPtr<nsIRunnable> gc_task = new IdleTimeGCTaskRunnable(js_runtime);
>+    NS_IdleDispatchToCurrentThread(gc_task.forget());
>+  }
What guarantees we don't end up dispatching more and more idle runnables before one gets chance to run?
Remember, if timeout value isn't passed to idle dispatch, nothing guarantees the runnable ever runs.

> 
>+  // Check whether we need an idle GC task.
>+  void CheckJSRuntimeState();
So the comment talks about idle gc task, but the method name doesn't hint about that at all.



[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

	Paul Bone [:pbone]
	Comment 36 • 7 years ago

(In reply to Jon Coppeard (:jonco) from comment #34)
>
> > +    /* Attemp to run a minor GC in the idle time if the free space falls below this threshold. */
> > +    static constexpr size_t kIdleTimeCollectionThreshold = 512 * 1024;  // 512 KB
> 
> This threshold is probably too large.  Most of the time the browser will
> have a 1MB nursery, and this could effectively halve this size.
> 
> I'd suggest making this 128KB.  We can always increase it later.

The nursary is sized in 1MB chunks on desktop (256KB on mobile) so I don't think it'd matter for desktop since the nursary can't get any smaller than 1MB.  But 128KB or 256KB make sense on mobile platforms and also work on desktop.

	Chia-Hung Duan Assignee
	Comment 38 • 7 years ago

(In reply to Olli Pettay [:smaug] from comment #35)
> Comment on attachment 8923321 [details] [diff] [review]
> Support idle runnable for nursery collection
> 
>  
> >+void CycleCollectedJSContext::CheckJSRuntimeState() {
> In Mozilla coding style[1] (which JS engine doesn't follow yet),  { goes to
> its own line with
> class and method declarations.
> 
> >+  // Get JSRuntime from CycleCollectedJSRuntime.
> >+  JSRuntime* js_runtime = Runtime()->Runtime();
> jsRuntime

Removed.

> 
> >+
> >+  class IdleTimeGCTaskRunnable : public mozilla::IdleRunnable {
> { to its own line
> 
> >+  public:
> >+    explicit IdleTimeGCTaskRunnable(JSRuntime* rt) : rt_(rt) { }
> Arguments should be named aName, so
> JSRuntime* aRt

Removed.

> 
> 
> >+  public:
> >+    NS_IMETHOD Run() override {
> { to its own line
> 
> >+      JS::RunIdleTimeGCTask(rt_);
> >+      return NS_OK;
> >+    }
> >+
> >+    nsresult Cancel() override {
> ditto

Done.

> 
> >+      return NS_OK;
> >+    }
> >+
> >+  private:
> >+    JSRuntime* rt_;
> JS_Runtime* mRt;
> What guarantees this never points to a deleted object? Is it guaranteed this
> code never runs during shutdown after runtime has been deleted.
> But I don't actually see the need for this. Run() method could always just
> access the current runtime.
> Something like
> CycleCollectedJSRuntime* ccrt = CycleCollectedJSRuntime::Get();
> if (ccrt && ccrt->Runtime()) {
>   JS::RunIdleTimeGCTask(ccrt->Runtime());
> }

Done.

> 
> 
> >+  };
> >+
> >+  if (JS::IsIdleGCTaskNeeded(js_runtime)) {
> >+    nsCOMPtr<nsIRunnable> gc_task = new IdleTimeGCTaskRunnable(js_runtime);
> >+    NS_IdleDispatchToCurrentThread(gc_task.forget());
> >+  }
> What guarantees we don't end up dispatching more and more idle runnables
> before one gets chance to run?
> Remember, if timeout value isn't passed to idle dispatch, nothing guarantees
> the runnable ever runs.

Done.
I forgot this, thanks!

> 
> > 
> >+  // Check whether we need an idle GC task.
> >+  void CheckJSRuntimeState();
> So the comment talks about idle gc task, but the method name doesn't hint
> about that at all.

Fix to IsIdleGCTaskNeeded.

> 
> 
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

	Chia-Hung Duan Assignee
	Comment 39 • 7 years ago

(In reply to Paul Bone [:pbone] from comment #36)
> (In reply to Jon Coppeard (:jonco) from comment #34)
> >
> > > +    /* Attemp to run a minor GC in the idle time if the free space falls below this threshold. */
> > > +    static constexpr size_t kIdleTimeCollectionThreshold = 512 * 1024;  // 512 KB
> > 
> > This threshold is probably too large.  Most of the time the browser will
> > have a 1MB nursery, and this could effectively halve this size.
> > 
> > I'd suggest making this 128KB.  We can always increase it later.
> 
> The nursary is sized in 1MB chunks on desktop (256KB on mobile) so I don't
> think it'd matter for desktop since the nursary can't get any smaller than
> 1MB.  But 128KB or 256KB make sense on mobile platforms and also work on
> desktop.

Thanks for reminding me about the mobile platform.

I use "kIdleTimeCollectionThreshold = NurseryChunkUsableSize / 4" now. What do you think?

	Jon Coppeard (:jonco) Reporter
	Comment 40 • 7 years ago

(In reply to Paul Bone [:pbone] from comment #36)
> The nursary is sized in 1MB chunks on desktop (256KB on mobile) so I don't
> think it'd matter for desktop since the nursary can't get any smaller than
> 1MB.  But 128KB or 256KB make sense on mobile platforms and also work on
> desktop.

Did I misunderstand what's going on here?  This is a threshold for the free space in the nursery, which is measured in bytes and not multiples of chunk size.

(In reply to Chia-Hung Duan from comment #39)
> I use "kIdleTimeCollectionThreshold = NurseryChunkUsableSize / 4" now. What
> do you think?

Yes, that sounds fine.

	Chia-Hung Duan Assignee
	Comment 42 • 7 years ago

Comment on attachment 8924047 [details] [diff] [review]
Support idle runnable for nursery collection. r=jonco,smaug

Rebase only. r+ carried forward

	Paul Bone [:pbone]
	Comment 43 • 7 years ago

(In reply to Jon Coppeard (:jonco) from comment #40)
> (In reply to Paul Bone [:pbone] from comment #36)
> > The nursary is sized in 1MB chunks on desktop (256KB on mobile) so I don't
> > think it'd matter for desktop since the nursary can't get any smaller than
> > 1MB.  But 128KB or 256KB make sense on mobile platforms and also work on
> > desktop.
> 
> Did I misunderstand what's going on here?  This is a threshold for the free
> space in the nursery, which is measured in bytes and not multiples of chunk
> size.

No you didn't misunderstand. I meant that with regards to your concern that a threshold of 1MB would create aggressive collection that would cause the nursery to be made smaller.  I'm just saying that this is not really a problem, it can't get smaller than 1MB (on desktop) since that's the chunk size.  I like Chia-Hung's new solution better anyway.

> (In reply to Chia-Hung Duan from comment #39)
> > I use "kIdleTimeCollectionThreshold = NurseryChunkUsableSize / 4" now. What
> > do you think?
> 
> Yes, that sounds fine.

I also agree.

	Pulsebot
	Comment 44 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf84e79c44c
Support idle runnable for nursery collection. r=jonco, r=smaug

	Natalia Csoregi [:nataliaCs]
	Comment 45 • 7 years ago

Backed out for bustage CycleCollectedJSRuntime.h:254

Failure log : https://treeherder.mozilla.org/logviewer.html#?job_id=141330252&repo=mozilla-inbound&lineNumber=9524

Push that caused the failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fbf84e79c44c07102797f8f53694cb7481a982d6

Back out: https://hg.mozilla.org/integration/mozilla-inbound/rev/60be9c0a0961438e091f7c730e8a29868fd644c0

	Chia-Hung Duan Assignee
	Comment 46 • 7 years ago

Sorry for missing const qualifier, fixed.

<none>

	Chia-Hung Duan Assignee
	Comment 47 • 7 years ago

Comment on attachment 8924450 [details] [diff] [review]
Support idle runnable for nursery collection r=jonco,smaug

minor syntax fixing, r+ carried forward

	Pulsebot
	Comment 48 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5877e17e1b98
Support idle runnable for nursery collection. r=jonco, r=smaug

	Narcis Beleuzu [:NarcisB]
	Comment 49 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5877e17e1b98