Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592703002:
abstract_code: return compiled code for compiled shared funcs (Closed)
|
Created:
4 years ago by Leszek Swirski Modified:
3 years, 11 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Descriptionabstract_code: return compiled code for compiled shared funcs
SharedFunctionInfo's abstract_code was returning the bytecode array
whenever SharedFunctionInfo had a bytecode array, even if the function
was compiled (e.g. tiered up to FCG). This meant that abstract_code
could return code that is not actually the code that will run, which was
causing problems in profiling as the sampled PC did not match the known
code offset.
This patch changes both SharedFunctionInfo and JSFunction to return the
bytecode if-and-only-if they are not compiled and have a bytecode array
to return, or they already point to the interpreter trampoline.
BUG=v8:5758
Review-Url: https://codereview.chromium.org/2592703002
Cr-Commit-Position: refs/heads/master@{#41894}
Committed: https://chromium.googlesource.com/v8/v8/+/679b31c21425ecdd86c40a8a02ac131b72f7bc6b
Patch Set 1 #Patch Set 2 : Add missing negation #Messages
Total messages: 23 (13 generated)
The CQ bit was checked by leszeks@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...
Hi all, I've added a bunch of reviewers for this because it's a behavioral change and I may be missing some consequences.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by leszeks@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.
Sounds reasonable to me. LGTM from my side.
LGTM.
The CQ bit was checked by leszeks@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482333150857900,
"parent_rev": "93df094081f04c629c3df2e40318de90ce5e0fb9", "commit_rev":
"679b31c21425ecdd86c40a8a02ac131b72f7bc6b"}
Message was sent while issue was closed.
Description was changed from ========== abstract_code: return compiled code for compiled shared funcs SharedFunctionInfo's abstract_code was returning the bytecode array whenever SharedFunctionInfo had a bytecode array, even if the function was compiled (e.g. tiered up to FCG). This meant that abstract_code could return code that is not actually the code that will run, which was causing problems in profiling as the sampled PC did not match the known code offset. This patch changes both SharedFunctionInfo and JSFunction to return the bytecode if-and-only-if they are not compiled and have a bytecode array to return, or they already point to the interpreter trampoline. BUG=v8:5758 ========== to ========== abstract_code: return compiled code for compiled shared funcs SharedFunctionInfo's abstract_code was returning the bytecode array whenever SharedFunctionInfo had a bytecode array, even if the function was compiled (e.g. tiered up to FCG). This meant that abstract_code could return code that is not actually the code that will run, which was causing problems in profiling as the sampled PC did not match the known code offset. This patch changes both SharedFunctionInfo and JSFunction to return the bytecode if-and-only-if they are not compiled and have a bytecode array to return, or they already point to the interpreter trampoline. BUG=v8:5758 Review-Url: https://codereview.chromium.org/2592703002 Cr-Commit-Position: refs/heads/master@{#41894} Committed: https://chromium.googlesource.com/v8/v8/+/679b31c21425ecdd86c40a8a02ac131b72f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/679b31c21425ecdd86c40a8a02ac131b72f...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2591223002/ by leszeks@chromium.org. The reason for reverting is: Breaks tree: http://build.chromium.org/p/client.v8/builders/V8%20Linux64%20GC%20Stress%20-....
Message was sent while issue was closed.
rmcilroy@chromium.org changed reviewers: + yangguo@chromium.org
Message was sent while issue was closed.
I'm not sure about this change. I think it will impact the debugger, which previously would get bytecode but now might get bytecode or FCG code depending on whether the function tiered up (adding Yang who should know if that's an issue). Also, it seems a bit confusing that what you get back depends on timing. How about instead renaming abstract_code to unoptized_code and add a new function to get baseline_code which returns the baseline code (but only if HasBaselineCode, so callers would need to check that first).
Message was sent while issue was closed.
On 2016/12/21 15:49:57, rmcilroy (OOO) wrote: > I'm not sure about this change. I think it will impact the debugger, which > previously would get bytecode but now might get bytecode or FCG code depending > on whether the function tiered up (adding Yang who should know if that's an > issue). Isn't that good though? You get back the thing that is actually running (or at least will run). > Also, it seems a bit confusing that what you get back depends on timing. That is already true in JSFunction. > How about instead renaming abstract_code to unoptized_code and add a new > function to get baseline_code which returns the baseline code (but only if > HasBaselineCode, so callers would need to check that first). Not sure about the naming here -- abstract_code, as written, will return optimised code for JSFunction. But, splitting the behaviour doesn't sound like a bad idea, it's possible that the debugger and profiler need different functions.
Message was sent while issue was closed.
On 2016/12/21 15:54:51, Leszek Swirski wrote: > On 2016/12/21 15:49:57, rmcilroy (OOO) wrote: > > I'm not sure about this change. I think it will impact the debugger, which > > previously would get bytecode but now might get bytecode or FCG code depending > > on whether the function tiered up (adding Yang who should know if that's an > > issue). > > Isn't that good though? You get back the thing that is actually running (or at > least > will run). > > > Also, it seems a bit confusing that what you get back depends on timing. > > That is already true in JSFunction. > > > How about instead renaming abstract_code to unoptized_code and add a new > > function to get baseline_code which returns the baseline code (but only if > > HasBaselineCode, so callers would need to check that first). > > Not sure about the naming here -- abstract_code, as written, will return > optimised > code for JSFunction. But, splitting the behaviour doesn't sound like a bad idea, > it's > possible that the debugger and profiler need different functions. should be fine. the debugger should be able to deal with either. but I think once we switch to TF+I we wont have to worry about any of this anymore.
Message was sent while issue was closed.
Looking at an alternative fix in https://codereview.chromium.org/2603333002 after f2f with Ross. |
