Last Comment Bug 725768 - BBP for ObjectHolders
: BBP for ObjectHolders
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Andrew Overholt [:overholt]
Depends on:
Blocks: 716598
  Show dependency treegraph
Reported: 2012-02-09 12:00 PST by Olli Pettay [:smaug]
Modified: 2012-03-29 15:18 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (4.11 KB, patch)
2012-02-09 12:00 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Splinter Review
additional patch (1011 bytes, patch)
2012-02-10 17:11 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Splinter Review
additional patch 2 (12.27 KB, patch)
2012-02-12 12:01 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Splinter Review
no default param value (13.13 KB, patch)
2012-02-13 13:20 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
all the patches for FF12 (16.91 KB, patch)
2012-02-23 10:35 PST, Olli Pettay [:smaug]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2012-02-09 12:00:14 PST
Created attachment 595838 [details] [diff] [review]
Comment 1 Olli Pettay [:smaug] 2012-02-10 06:39:47 PST
Comment on attachment 595838 [details] [diff] [review]

This relies on CanSkip to remove stuff from purple buffer and mark objects
black. This should be useful in cases when DOM Node is in CCGeneration document,
but not suspected.
Comment 2 Olli Pettay [:smaug] 2012-02-10 12:39:08 PST
Comment 3 Olli Pettay [:smaug] 2012-02-10 17:00:08 PST
Bah, I found a case when this is overkill. I need yet another CanSkip method, or
change the existing nsGenericElement::CanSkip.
Comment 4 Olli Pettay [:smaug] 2012-02-10 17:11:58 PST
Created attachment 596235 [details] [diff] [review]
additional patch

If we have a disconnected tree, make sure we know that is has been handled already.
This wasn't an issue before, since we'd use CanSkip only during
actual forgetSkippable call.
Comment 5 Olli Pettay [:smaug] 2012-02-10 17:26:39 PST
Comment 6 Olli Pettay [:smaug] 2012-02-11 03:53:37 PST
This is so effective, that if no regressions are found, we could perhaps think of taking this
to FF12.
Comment 7 Olli Pettay [:smaug] 2012-02-12 12:01:52 PST
Created attachment 596502 [details] [diff] [review]
additional patch 2

This isn't a huge problem, since object is usually removed from the purple buffer
when forgetSkippable next time runs. But that needs possibly a new
DOM tree traversing.

(I actually thought the situation was worse until I noticed that I was testing
a worse-case version of a testcase)
Comment 8 Alex Keybl [:akeybl] 2012-02-13 10:48:14 PST
Do we think this patch will help mitigate recent GC/CC issues? What's the motivation for uplifting?
Comment 9 Andrew McCreight [:mccr8] 2012-02-13 11:04:07 PST
Comment on attachment 596502 [details] [diff] [review]
additional patch 2

Review of attachment 596502 [details] [diff] [review]:

This is a little gross, but I guess we can leave untangling this unpurple stuff to a future patch.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +148,5 @@
>      // If CanSkip returns true, p is removed from the purple buffer during
>      // a call to nsCycleCollector_forgetSkippable().
>      // Note, calling CanSkip may remove objects from the purple buffer!
> +    // If aRemovingAllowed is true, p can be removed from the purple buffer.
> +    bool CanSkip(void *p, bool aRemovingAllowed = false)

CanSkip is only called in two places, one with true and one with false, so having a default argument doesn't make sense.
Comment 10 Olli Pettay [:smaug] 2012-02-13 12:56:41 PST
(In reply to Alex Keybl [:akeybl] from comment #8)
> Do we think this patch will help mitigate recent GC/CC issues? What's the
> motivation for uplifting?
This patch seems, at least locally cut down CC times significantly. Also, this shouldn't be
horribly risky patch (I know, there have been 2 followups).
This is not about mitigating recent CC issues, but actually part of fixing CC handling.
Most of the CC handling fixes did land to FF12.
Comment 11 Olli Pettay [:smaug] 2012-02-13 13:04:52 PST
But before getting this to FF12, it is better to wait for few days to get some more Telemetry data.
Comment 12 Olli Pettay [:smaug] 2012-02-13 13:20:26 PST
Created attachment 596774 [details] [diff] [review]
no default param value
Comment 13 Alex Keybl [:akeybl] 2012-02-23 09:43:02 PST
We're now reconsidering for uplift to Aurora given the conclusion to our recent CC investigations, which made it clear that we were seeing significantly better performance on FF13. Tracking for FF12.
Comment 14 Olli Pettay [:smaug] 2012-02-23 10:15:36 PST
I'll prepare patches for FF12
Comment 15 Olli Pettay [:smaug] 2012-02-23 10:35:12 PST
Created attachment 600083 [details] [diff] [review]
all the patches for FF12

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Higher CC times
Testing completed (on m-c, etc.): 2 weeks in m-c
Risk to taking this patch (and alternatives if risky): Should be reasonable low risk, since
  it is just using other optimizations used also elsewhere.
String changes made by this patch: N/A

Hopefully it even compiles :)
Comment 16 Alex Keybl [:akeybl] 2012-02-23 15:23:50 PST
Comment on attachment 600083 [details] [diff] [review]
all the patches for FF12

[Triage Comment]
Approving for Aurora 12 to help defend against some of the pauses we've had internal reports about.

Note You need to log in before you can comment on or make changes to this bug.