Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2026993004:
Delay leak reporting until worker in-process proxies have been finalized. (Closed)
|
Created:
4 years, 6 months ago by sof Modified:
4 years, 6 months ago CC:
chromium-reviews, falken, kinuko+worker_chromium.org, blink-reviews, horo+watch_chromium.org, kinuko+watch, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelay leak reporting until worker in-process proxies have been finalized.
If a document creates a number of workers, terminating these and having
their destruction ripple all the way back to the in-process proxy objects
isn't immediate. But something that needs to complete before the leak
detector can initiate reporting -- an in-process proxy object maintains a
strong reference to the document, and would generate a leak if not
destructed and its garbage having been collected afterwards.
Address the reliability of multi worker shutdown by maintaining a
counter of how many in-process proxy objects are still alive and run
GCs until it drops to zero. Do that at most two times around.
R=haraken,kouhei
BUG=589802, 616714
Committed: https://crrev.com/b0206d8c4aa1f5a659169111a722d183fc2e7739
Cr-Commit-Position: refs/heads/master@{#397405}
Patch Set 1 #Patch Set 2 : assume main thread usage #Patch Set 3 : use proxy count to decide if another GC round is reqd #Patch Set 4 : remove dated comment #Patch Set 5 : drop back to ps#2 approach #
Messages
Total messages: 33 (13 generated)
sigbjornf@opera.com changed reviewers: + haraken@chromium.org, kouhei@chromium.org
please take a look.
Just help me understand, why does the leak detector runs before the main thread shuts down all workers (and thus closes all messaging proxies between the workers and the main thread)?
On 2016/06/01 15:21:52, haraken wrote: > Just help me understand, why does the leak detector runs before the main thread > shuts down all workers (and thus closes all messaging proxies between the > workers and the main thread)? We do, see the call to terminateAndWaitForAllWorkers() in the initial preparation phase (line 86.) We do need to flush out all the garbage from that..and make sure all posted destruction tasks get to run (and their garbage again being released.)
On 2016/06/01 15:36:30, sof wrote: > On 2016/06/01 15:21:52, haraken wrote: > > Just help me understand, why does the leak detector runs before the main > thread > > shuts down all workers (and thus closes all messaging proxies between the > > workers and the main thread)? > > We do, see the call to terminateAndWaitForAllWorkers() in the initial > preparation phase (line 86.) > > We do need to flush out all the garbage from that..and make sure all posted > destruction tasks get to run (and their garbage again being released.) Maybe would it be better to always post a task for web_leak_detector_->collectGarbageAndReport() so that the task runs after all other tasks has been drained?
On 2016/06/02 01:12:14, haraken wrote: > On 2016/06/01 15:36:30, sof wrote: > > On 2016/06/01 15:21:52, haraken wrote: > > > Just help me understand, why does the leak detector runs before the main > > thread > > > shuts down all workers (and thus closes all messaging proxies between the > > > workers and the main thread)? > > > > We do, see the call to terminateAndWaitForAllWorkers() in the initial > > preparation phase (line 86.) > > > > We do need to flush out all the garbage from that..and make sure all posted > > destruction tasks get to run (and their garbage again being released.) > > Maybe would it be better to always post a task for > web_leak_detector_->collectGarbageAndReport() so that the task runs after all > other tasks has been drained? But we have no way of knowing when all the workers have finished notifying their proxies of termination, so the point at which we can safely add that final task isn't known.
On 2016/06/02 05:27:14, sof wrote: > On 2016/06/02 01:12:14, haraken wrote: > > On 2016/06/01 15:36:30, sof wrote: > > > On 2016/06/01 15:21:52, haraken wrote: > > > > Just help me understand, why does the leak detector runs before the main > > > thread > > > > shuts down all workers (and thus closes all messaging proxies between the > > > > workers and the main thread)? > > > > > > We do, see the call to terminateAndWaitForAllWorkers() in the initial > > > preparation phase (line 86.) > > > > > > We do need to flush out all the garbage from that..and make sure all posted > > > destruction tasks get to run (and their garbage again being released.) > > > > Maybe would it be better to always post a task for > > web_leak_detector_->collectGarbageAndReport() so that the task runs after all > > other tasks has been drained? > > But we have no way of knowing when all the workers have finished notifying their > proxies of termination, so the point at which we can safely add that final task > isn't known. Thanks, makes sense. LGTM.
lgtm
Hmm, I think we can do better now that we have this proxy count available -- if it reaches 0 (which it will be for all non-worker tests), we don't need to perform the extra GC iteration => faster leak detection. Let me try.
On 2016/06/02 05:46:22, sof wrote: > Hmm, I think we can do better now that we have this proxy count available -- if > it reaches 0 (which it will be for all non-worker tests), we don't need to > perform the extra GC iteration => faster leak detection. > > Let me try. Worked out, I think - ptal?
LGTM
still lgtm
Description was changed from ========== Delay leak reporting until worker in-process proxies have been finalized. If a GC ends up releasing a larger number of in-process worker objects at the same time, it was possible that all posted tasks for finalizing them and their in-process proxies would not all get to run before the next (and final) round of GCs did, leading to leaks being (flakily) reported. Address this by tracking the number of in-process proxies that are still around; if there are any left when the leak detector considers itself done with tidying GCs, do another round of GCs, but not before the posted finalizing tasks have run. R= BUG=589802 ========== to ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802 ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026993004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2026993004/60001
Message was sent while issue was closed.
Description was changed from ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802 ========== to ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802 ========== to ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802 Committed: https://crrev.com/19895c52589f189d6fdc563759c9872af2cbf86c Cr-Commit-Position: refs/heads/master@{#397333} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/19895c52589f189d6fdc563759c9872af2cbf86c Cr-Commit-Position: refs/heads/master@{#397333}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2030883003/ by vabr@chromium.org. The reason for reverting is: Speculative revert, seems to have broken some content_browsertests. See the bug: BUG=616714.
Message was sent while issue was closed.
Description was changed from ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802 Committed: https://crrev.com/19895c52589f189d6fdc563759c9872af2cbf86c Cr-Commit-Position: refs/heads/master@{#397333} ========== to ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802 Committed: https://crrev.com/19895c52589f189d6fdc563759c9872af2cbf86c Cr-Commit-Position: refs/heads/master@{#397333} ==========
Going back to what ps#2 did, as chasing after reported leaks in unit tests that do not reproduce locally isn't worth sinking more time into.
Description was changed from ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802 Committed: https://crrev.com/19895c52589f189d6fdc563759c9872af2cbf86c Cr-Commit-Position: refs/heads/master@{#397333} ========== to ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802, 616714 Committed: https://crrev.com/19895c52589f189d6fdc563759c9872af2cbf86c Cr-Commit-Position: refs/heads/master@{#397333} ==========
Description was changed from ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802, 616714 Committed: https://crrev.com/19895c52589f189d6fdc563759c9872af2cbf86c Cr-Commit-Position: refs/heads/master@{#397333} ========== to ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802, 616714 ==========
Description was changed from ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. For documents not creating any workers, monitoring this proxy count avoids having to perform a third GC, something that was conservatively done before to address worker shutdown (but not reliably.) R=haraken,kouhei BUG=589802, 616714 ========== to ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. R=haraken,kouhei BUG=589802, 616714 ==========
LGTM
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2026993004/#ps80001 (title: "drop back to ps#2 approach")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026993004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2026993004/80001
Message was sent while issue was closed.
Description was changed from ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. R=haraken,kouhei BUG=589802, 616714 ========== to ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. R=haraken,kouhei BUG=589802, 616714 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. R=haraken,kouhei BUG=589802, 616714 ========== to ========== Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. R=haraken,kouhei BUG=589802, 616714 Committed: https://crrev.com/b0206d8c4aa1f5a659169111a722d183fc2e7739 Cr-Commit-Position: refs/heads/master@{#397405} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b0206d8c4aa1f5a659169111a722d183fc2e7739 Cr-Commit-Position: refs/heads/master@{#397405} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
