Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1950613005:
Fixes tests that use internals.observeGC to work with Ignition. (Closed)
DescriptionFixes tests that use internals.observeGC to work with Ignition.
In Ignition references to objects may remain live on the function's stack frame.
This may prevent GC from collective otherwise dead objects. The objects passed to
internals.ObserveGC function may remain live on the function's stack preventing
GC from collecting them. To avoid this, parameters are not passed directly to this
function. Instead, an inner function is used to access these objects.
This will avoid having references to object on this function's stack.
BUG=chromium:595672, v8:4280
Committed: https://crrev.com/00f22678f26f4548e5b748ab4ae57a996ec41323
Cr-Commit-Position: refs/heads/master@{#394130}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added a wrapper for observeGC. #Patch Set 3 : fixed line couloum width. #
Total comments: 2
Patch Set 4 : Fixes comments and fixes more tests. #
Total comments: 4
Patch Set 5 : Address comments. And updates other internals.observeGC to observeGC. #Patch Set 6 : Rebased the patch #Patch Set 7 : updates test expectations. #Patch Set 8 : Removed special wrapper for internals.observeGC. Adds an inner function to pass the object. #Messages
Total messages: 42 (21 generated)
mythria@chromium.org changed reviewers: + rmcilroy@chromium.org
Ross, if these fixes look ok, I will also fix other failing tests along the same lines. Thanks, Mythri
https://codereview.chromium.org/1950613005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html (right): https://codereview.chromium.org/1950613005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html:17: firstRectForCharacterRangeGC = internals.observeGC(textInputController.firstRectForCharacterRange(0, 0)); What actually lives in a temporary register and needs to be cleared here before the GC for it to work? Presumably just the arguments to the observeGC calls. Could we do something more clever by changing the observeGC API to ensure that the arguments are never maintained in dead registers?
PTAL. If this looks reasonable, I will update other tests as well. I am planning to replace all observeGC's with callObserveGC even if they don't cause a problem now.
Approach looks good, couple of comments. https://codereview.chromium.org/1950613005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html (right): https://codereview.chromium.org/1950613005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html:12: return textInputController.markedRange();}); nit - '});' on newline. https://codereview.chromium.org/1950613005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/resources/observeGCWrapper.js (right): https://codereview.chromium.org/1950613005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resources/observeGCWrapper.js:1: function callObserveGC(f) { I'd just call this observeGC() and name the JS file observeGC.js. Also, please add a comment explaining why what this does (i.e., returns a GC observer for the object which is returned by function f) and why internals.observeGC shouldn't be used directly. Also a better name for 'f' would be good.
Description was changed from ========== Updated few layout tests to work with ignition In ignition dead temporary registers may hold references to objects preventing GC from collecting them. It is expensive to clear the dead registers and there is no noticable impact on heap size. This cl updates some of the layout tests to work with ignition. BUG=chromium:595672,v8:4280 ========== to ========== Adds observeGC function that accepts a function. In ignition dead temporary registers may hold references to objects preventing GC from collecting them. It is expensive to clear the dead registers and there is no noticable impact on heap size. internals.ObserveGC function is used to check if an object is alive after a GC. Since this can potentially cause problems with ignition, a wrapper function is introduced to work around this problem. BUG=chromium:595672,v8:4280 ==========
Description was changed from ========== Adds observeGC function that accepts a function. In ignition dead temporary registers may hold references to objects preventing GC from collecting them. It is expensive to clear the dead registers and there is no noticable impact on heap size. internals.ObserveGC function is used to check if an object is alive after a GC. Since this can potentially cause problems with ignition, a wrapper function is introduced to work around this problem. BUG=chromium:595672,v8:4280 ========== to ========== Adds observeGC function that accepts a function. In ignition dead temporary registers may hold references to objects preventing GC from collecting them. internals.ObserveGC function is used to check if an object is alive after a GC. Since this can potentially cause problems with ignition, a wrapper function is introduced that accepts a function that returns the object to be observed. Since registers are released on function return this avoids holding references in dead registers BUG=chromium:595672,v8:4280 ==========
Description was changed from ========== Adds observeGC function that accepts a function. In ignition dead temporary registers may hold references to objects preventing GC from collecting them. internals.ObserveGC function is used to check if an object is alive after a GC. Since this can potentially cause problems with ignition, a wrapper function is introduced that accepts a function that returns the object to be observed. Since registers are released on function return this avoids holding references in dead registers BUG=chromium:595672,v8:4280 ========== to ========== Adds observeGC function that accepts a function. In ignition dead temporary registers may hold references to objects preventing GC from collecting them. internals.ObserveGC function is used to check if an object is alive after a GC. Since this can potentially cause problems with ignition, a wrapper function is introduced that accepts a function that returns the object to be observed. Since registers are released on function return this avoids holding references in dead registers BUG=chromium:595672,v8:4280 ==========
Description was changed from ========== Adds observeGC function that accepts a function. In ignition dead temporary registers may hold references to objects preventing GC from collecting them. internals.ObserveGC function is used to check if an object is alive after a GC. Since this can potentially cause problems with ignition, a wrapper function is introduced that accepts a function that returns the object to be observed. Since registers are released on function return this avoids holding references in dead registers BUG=chromium:595672,v8:4280 ========== to ========== Adds a wrapper for internals.observeGC to fix tests for ignition. In ignition dead temporary registers may hold references to objects preventing GC from collecting them. internals.ObserveGC function is used to check if an object is alive after a GC. Since this can potentially cause problems with ignition, a wrapper function is introduced that accepts a function that returns the object to be observed. Since registers are released on function return this avoids holding references in dead registers BUG=chromium:595672,v8:4280 ==========
https://codereview.chromium.org/1950613005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/resources/observeGC.js (right): https://codereview.chromium.org/1950613005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resources/observeGC.js:1: // Returns a GCObservation object for the object returned by the Add copyright notice. https://codereview.chromium.org/1950613005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resources/observeGC.js:5: // might be held in a dead register, preventing GC from collecting it. To avoid The "register" terminology is to technical to understand here. Just talk about temporary expression values remaining on the stack frame, which could keep the object alive." Also, add newlines between the description of how to use the function and the description of why it's necessary.
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950613005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950613005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950613005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950613005/100001
Thanks for your review. I fixed them. I also changed internals.observeGC calls to observeGC calls in other places that are not causing a problem currently. PTAL. https://codereview.chromium.org/1950613005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/resources/observeGC.js (right): https://codereview.chromium.org/1950613005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resources/observeGC.js:1: // Returns a GCObservation object for the object returned by the On 2016/05/13 11:21:08, rmcilroy wrote: > Add copyright notice. As discussed offline. We don't need it here. https://codereview.chromium.org/1950613005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resources/observeGC.js:5: // might be held in a dead register, preventing GC from collecting it. To avoid On 2016/05/13 11:21:08, rmcilroy wrote: > The "register" terminology is to technical to understand here. Just talk about > temporary expression values remaining on the stack frame, which could keep the > object alive." > > Also, add newlines between the description of how to use the function and the > description of why it's necessary. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950613005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950613005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. I updated test expectations of two benchmarks: storage/indexeddb/request-leak-expected.txt and fast/harness/internals-observe-gc-expected.txt.
As discussed offline, let's not add the observeGC wrapper and instead just wrap calls in an inner function where necessary with a comment as to why this is necessary.
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950613005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950613005/140001
Description was changed from ========== Adds a wrapper for internals.observeGC to fix tests for ignition. In ignition dead temporary registers may hold references to objects preventing GC from collecting them. internals.ObserveGC function is used to check if an object is alive after a GC. Since this can potentially cause problems with ignition, a wrapper function is introduced that accepts a function that returns the object to be observed. Since registers are released on function return this avoids holding references in dead registers BUG=chromium:595672,v8:4280 ========== to ========== Fixes tests that use internals.observeGC to work with ignition. In ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. internals.ObserveGC function is used to check if an object is alive after a GC. Since this can potentially cause problems with ignition, the objects to the observeGC are not passed directly. Instead they are passed by making a function return the object. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ==========
Description was changed from ========== Fixes tests that use internals.observeGC to work with ignition. In ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. internals.ObserveGC function is used to check if an object is alive after a GC. Since this can potentially cause problems with ignition, the objects to the observeGC are not passed directly. Instead they are passed by making a function return the object. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ========== to ========== Fixes tests that use internals.observeGC to work with ignition. In ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead they are passed by making a function return the object. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ==========
Description was changed from ========== Fixes tests that use internals.observeGC to work with ignition. In ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead they are passed by making a function return the object. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ========== to ========== Fixes tests that use internals.observeGC to work with ignition. In ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead they are passed by making a function return the object. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ==========
Description was changed from ========== Fixes tests that use internals.observeGC to work with ignition. In ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead they are passed by making a function return the object. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ========== to ========== Fixes tests that use internals.observeGC to work with ignition. In ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead, an inner function is used to access these objects. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ==========
PTAL.
Description was changed from ========== Fixes tests that use internals.observeGC to work with ignition. In ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead, an inner function is used to access these objects. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ========== to ========== Fixes tests that use internals.observeGC to work with Ignition. In Ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead, an inner function is used to access these objects. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, thanks.
The CQ bit was checked by mythria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950613005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950613005/140001
Message was sent while issue was closed.
Description was changed from ========== Fixes tests that use internals.observeGC to work with Ignition. In Ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead, an inner function is used to access these objects. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ========== to ========== Fixes tests that use internals.observeGC to work with Ignition. In Ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead, an inner function is used to access these objects. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fixes tests that use internals.observeGC to work with Ignition. In Ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead, an inner function is used to access these objects. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 ========== to ========== Fixes tests that use internals.observeGC to work with Ignition. In Ignition references to objects may remain live on the function's stack frame. This may prevent GC from collective otherwise dead objects. The objects passed to internals.ObserveGC function may remain live on the function's stack preventing GC from collecting them. To avoid this, parameters are not passed directly to this function. Instead, an inner function is used to access these objects. This will avoid having references to object on this function's stack. BUG=chromium:595672,v8:4280 Committed: https://crrev.com/00f22678f26f4548e5b748ab4ae57a996ec41323 Cr-Commit-Position: refs/heads/master@{#394130} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/00f22678f26f4548e5b748ab4ae57a996ec41323 Cr-Commit-Position: refs/heads/master@{#394130} |
