Closed Bug 725768 Opened 12 years ago Closed 12 years ago

BBP for ObjectHolders

Categories

(Core :: XPConnect, defect)

13 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox12 + fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Whiteboard: [snappy][qa-])

Attachments

(5 files)

Attached patch patchSplinter Review
      No description provided.
Blocks: 705582
OS: Linux → All
Hardware: x86_64 → All
Blocks: 716598
No longer blocks: 705582
Comment on attachment 595838 [details] [diff] [review]
patch

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.
Attachment #595838 - Flags: review?(continuation)
Attachment #595838 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/04b311e4bc94
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Bah, I found a case when this is overkill. I need yet another CanSkip method, or
change the existing nsGenericElement::CanSkip.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch additional patchSplinter Review
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.
Attachment #596235 - Flags: review?(continuation)
Attachment #596235 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/c8aee2ac4eba
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
This is so effective, that if no regressions are found, we could perhaps think of taking this
to FF12.
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)
Attachment #596502 - Flags: review?(continuation)
Do we think this patch will help mitigate recent GC/CC issues? What's the motivation for uplifting?
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.
Attachment #596502 - Flags: review?(continuation) → review+
(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.
But before getting this to FF12, it is better to wait for few days to get some more Telemetry data.
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.
Version: 12 Branch → 13 Branch
I'll prepare 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 :)
https://tbpl.mozilla.org/?tree=Try&rev=d8038c33230e
Attachment #600083 - Flags: approval-mozilla-aurora?
Whiteboard: [snappy]
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.
Attachment #600083 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [snappy] → [snappy][qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: