Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2450233002:
Check if the frame is optimized before marking a function for optimization. (Closed)
|
Created:
4 years, 1 month ago by mythria Modified:
4 years, 1 month ago Reviewers:
Michael Starzinger CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionCheck if the frame is optimized before marking a function for optimization.
When checking for marking a function for optimization, we had a check if
the function is already optimized to return early. This works in non-OSR
cases. For Turbofan OSR even when the current execution of the function
has already been optimized, the function itself will not be replaced
with optimized code. Hence, we may end up checking a function that is
already marked for optimization again. A check for the frame being optimized
avoids these checks.
BUG=
Committed: https://crrev.com/8daff84d86979be2d06f9f4ba03e6009dd047212
Cr-Commit-Position: refs/heads/master@{#40760}
Patch Set 1 #
Total comments: 4
Patch Set 2 : removed check for FLAG_ignition_OSR from MaybeOSRIgnition. #Messages
Total messages: 18 (12 generated)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11517) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
mythria@chromium.org changed reviewers: + mstarzinger@chromium.org
Hi Michi, this is the cl to change checks from function being optimized to frame being optimized. This is not working yet. I have added my comments inline. Thanks, Mythri https://codereview.chromium.org/2450233002/diff/1/src/runtime-profiler.cc File src/runtime-profiler.cc (right): https://codereview.chromium.org/2450233002/diff/1/src/runtime-profiler.cc#new... src/runtime-profiler.cc:344: } else if (MaybeOSRIgnition(function, frame)) { when FLAG_ignition_osr is disabled MaybeOSRIgnition just returns false. So we will execute the later part of the function even when the function is optimized. When I change the check from function->IsOptimized to frame->is_optimized(), we may end up marking optimized functions for optimization again if we haven't moved to the optimized code yet. So we have three choices here: 1. Remove the FLAG_ignition_osr check in MaybeOSRIgnition. Since this is already handled in AttemptOnStackReplacement. 2. Also add a check for function->IsOptimized() where we check for optimized frames. 3. Just retain the check for optimized functions. I listed them in my order of preference. Let me know what you think would be the best. https://codereview.chromium.org/2450233002/diff/1/src/runtime-profiler.cc#new... src/runtime-profiler.cc:374: if (!FLAG_ignition_osr) return false; This is the check I was referring to in my earlier comment.
LGTM. https://codereview.chromium.org/2450233002/diff/1/src/runtime-profiler.cc File src/runtime-profiler.cc (right): https://codereview.chromium.org/2450233002/diff/1/src/runtime-profiler.cc#new... src/runtime-profiler.cc:344: } else if (MaybeOSRIgnition(function, frame)) { On 2016/10/31 16:02:03, mythria wrote: > when FLAG_ignition_osr is disabled MaybeOSRIgnition just returns false. So we > will execute the later part of the function even when the function is optimized. > When I change the check from function->IsOptimized to frame->is_optimized(), we > may end up marking optimized functions for optimization again if we haven't > moved to the optimized code yet. So we have three choices here: > > 1. Remove the FLAG_ignition_osr check in MaybeOSRIgnition. Since this is already > handled in AttemptOnStackReplacement. > > 2. Also add a check for function->IsOptimized() where we check for optimized > frames. > > 3. Just retain the check for optimized functions. > > I listed them in my order of preference. Let me know what you think would be the > best. I would also prefer (1), especially given that we also want to enable --ignition-osr by default anyways. See: https://codereview.chromium.org/2432413004/
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Michi. I removed the check on the flag. https://codereview.chromium.org/2450233002/diff/1/src/runtime-profiler.cc File src/runtime-profiler.cc (right): https://codereview.chromium.org/2450233002/diff/1/src/runtime-profiler.cc#new... src/runtime-profiler.cc:344: } else if (MaybeOSRIgnition(function, frame)) { On 2016/11/03 11:40:27, Michael Starzinger wrote: > On 2016/10/31 16:02:03, mythria wrote: > > when FLAG_ignition_osr is disabled MaybeOSRIgnition just returns false. So we > > will execute the later part of the function even when the function is > optimized. > > When I change the check from function->IsOptimized to frame->is_optimized(), > we > > may end up marking optimized functions for optimization again if we haven't > > moved to the optimized code yet. So we have three choices here: > > > > 1. Remove the FLAG_ignition_osr check in MaybeOSRIgnition. Since this is > already > > handled in AttemptOnStackReplacement. > > > > 2. Also add a check for function->IsOptimized() where we check for optimized > > frames. > > > > 3. Just retain the check for optimized functions. > > > > I listed them in my order of preference. Let me know what you think would be > the > > best. > > I would also prefer (1), especially given that we also want to enable > --ignition-osr by default anyways. See: > https://codereview.chromium.org/2432413004/ Done.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2450233002/#ps20001 (title: "removed check for FLAG_ignition_OSR from MaybeOSRIgnition.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Check if the frame is optimized before marking a function for optimization. When checking for marking a function for optimization, we had a check if the function is already optimized to return early. This works in non-OSR cases. For Turbofan OSR even when the current execution of the function has already been optimized, the function itself will not be replaced with optimized code. Hence, we may end up checking a function that is already marked for optimization again. A check for the frame being optimized avoids these checks. BUG= ========== to ========== Check if the frame is optimized before marking a function for optimization. When checking for marking a function for optimization, we had a check if the function is already optimized to return early. This works in non-OSR cases. For Turbofan OSR even when the current execution of the function has already been optimized, the function itself will not be replaced with optimized code. Hence, we may end up checking a function that is already marked for optimization again. A check for the frame being optimized avoids these checks. BUG= Committed: https://crrev.com/8daff84d86979be2d06f9f4ba03e6009dd047212 Cr-Commit-Position: refs/heads/master@{#40760} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8daff84d86979be2d06f9f4ba03e6009dd047212 Cr-Commit-Position: refs/heads/master@{#40760} |
