The default bug view has changed. See this FAQ.

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug)

13 Branch
Points:
---

Firefox Tracking Flags

(firefox12+ fixed)

Details

(Whiteboard: [snappy][qa-])

Attachments

(5 attachments)

(Assignee)

Description

5 years ago
Created attachment 595838 [details] [diff] [review]
patch
Blocks: 705582
OS: Linux → All
Hardware: x86_64 → All
Blocks: 716598
No longer blocks: 705582
(Assignee)

Comment 1

5 years ago
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+
(Assignee)

Comment 2

5 years ago
https://hg.mozilla.org/mozilla-central/rev/04b311e4bc94
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 3

5 years ago
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 → ---
(Assignee)

Comment 4

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

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/c8aee2ac4eba
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

5 years ago
This is so effective, that if no regressions are found, we could perhaps think of taking this
to FF12.
tracking-firefox12: --- → ?
(Assignee)

Comment 7

5 years ago
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)
Attachment #596502 - Flags: review?(continuation)

Comment 8

5 years ago
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+
(Assignee)

Comment 10

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

Comment 11

5 years ago
But before getting this to FF12, it is better to wait for few days to get some more Telemetry data.
(Assignee)

Comment 12

5 years ago
Created attachment 596774 [details] [diff] [review]
no default param value

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

Updated

5 years ago
tracking-firefox12: ? → -
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.
tracking-firefox12: - → +
Version: 12 Branch → 13 Branch
(Assignee)

Comment 14

5 years ago
I'll prepare patches for FF12
(Assignee)

Comment 15

5 years ago
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 :)
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+
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/856dfbcebb26
status-firefox12: --- → fixed
Whiteboard: [snappy] → [snappy][qa-]
You need to log in before you can comment on or make changes to this bug.