Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1407513003:
[Interpreter]: Basic support for iterating interpreter stack frames for GC. (Closed)
Description[Interpreter]: Basic support for iterating interpreter stack frames for GC.
Adds basic support for iterating interpreter stack frames for GC. Currently
InterpreterStackFrames are treated just like JavaScriptStackFrames since the
JavaScriptFrame::IterateExpressions() will correctly iterate over all the
local / temp interpeter Registers, and will iterate over the
interpreter_entry_trampoline pc address. There is no need to explicitly
iterate over the BytecodeArray object since that is held in a machine
register in the bytecode handler which is marked as kMachTaggedAny by
TurboFan, and so will get iterated appropriately when iterating the
bytecode handler stub's stack frame.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/4b2fffae4c42f0e95333f1f3466c5d8ba7ee4585
Cr-Commit-Position: refs/heads/master@{#31342}
Patch Set 1 : #
Total comments: 4
Messages
Total messages: 27 (7 generated)
Patchset #1 (id:1) has been deleted
rmcilroy@chromium.org changed reviewers: + titzer@chromium.org
This adds basic GC iteration support - exception printing / stackwalking isn't supported for interpreted frames yet. Ben: Looks like you were the last person looking at StackFrames::ComputeType - could you take a look please?
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by rmcilroy@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/1407513003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407513003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
oth@chromium.org changed reviewers: + oth@chromium.org
Looks fine here. Pleasantly surprised at how small this turned out to be. https://codereview.chromium.org/1407513003/diff/40001/src/frames.h File src/frames.h (right): https://codereview.chromium.org/1407513003/diff/40001/src/frames.h#newcode107 src/frames.h:107: V(INTERPRETED, InterpretedFrame) \ Not asking for a change, but wonder about InterpreterFrame vs InterpretedFrame vs InterpretedCodeFrame. The former seems like it might show up more readily in seaches and is a noun/noun combo which seems most common here.
https://codereview.chromium.org/1407513003/diff/40001/src/frames.h File src/frames.h (right): https://codereview.chromium.org/1407513003/diff/40001/src/frames.h#newcode107 src/frames.h:107: V(INTERPRETED, InterpretedFrame) \ On 2015/10/14 12:19:32, oth wrote: > Not asking for a change, but wonder about InterpreterFrame vs InterpretedFrame > vs InterpretedCodeFrame. The former seems like it might show up more readily in > seaches and is a noun/noun combo which seems most common here. I was trying to keep this similar to OPTIMIZED / OptimizedFrame. I'm easy either way though - any preference Ben?
On 2015/10/14 14:18:47, rmcilroy wrote: > https://codereview.chromium.org/1407513003/diff/40001/src/frames.h > File src/frames.h (right): > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.h#newcode107 > src/frames.h:107: V(INTERPRETED, InterpretedFrame) \ > On 2015/10/14 12:19:32, oth wrote: > > Not asking for a change, but wonder about InterpreterFrame vs InterpretedFrame > > vs InterpretedCodeFrame. The former seems like it might show up more readily > in > > seaches and is a noun/noun combo which seems most common here. > > I was trying to keep this similar to OPTIMIZED / OptimizedFrame. I'm easy either > way though - any preference Ben? It can stay as is. Just noting these two differ from the other, the word code has been optimized out :-)
On 2015/10/14 14:51:19, oth wrote: > On 2015/10/14 14:18:47, rmcilroy wrote: > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.h > > File src/frames.h (right): > > > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.h#newcode107 > > src/frames.h:107: V(INTERPRETED, InterpretedFrame) \ > > On 2015/10/14 12:19:32, oth wrote: > > > Not asking for a change, but wonder about InterpreterFrame vs > InterpretedFrame > > > vs InterpretedCodeFrame. The former seems like it might show up more readily > > in > > > seaches and is a noun/noun combo which seems most common here. > > > > I was trying to keep this similar to OPTIMIZED / OptimizedFrame. I'm easy > either > > way though - any preference Ben? > > It can stay as is. Just noting these two differ from the other, the word code > has been optimized out :-) Mostly looks straightforward, but we have to be careful to make this robust.
https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 src/frames.cc:442: case Code::BUILTIN: Are interpreter frames really only identifiable through the marker being a SMI, or is there a more reliable way to detect them? E.g. by marking a bit on the code object?
https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 src/frames.cc:442: case Code::BUILTIN: On 2015/10/15 17:01:55, titzer wrote: > Are interpreter frames really only identifiable through the marker being a SMI, > or is there a more reliable way to detect them? E.g. by marking a bit on the > code object? The marker is not a SMI (it's a tagged JSFunction object, like other JavaScript frames). It is identified by the fact that the return address always points to the InterpreterEntryTrampoline builtin for interpreted frames. If you want to make it more robust I could do the same trick as the ArgumentsAdapter (put a SMI marker in the context slot and push the context separately. Would this be better? What was your plan for WASM frames?
On 2015/10/15 19:47:47, rmcilroy wrote: > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc > File src/frames.cc (right): > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 > src/frames.cc:442: case Code::BUILTIN: > On 2015/10/15 17:01:55, titzer wrote: > > Are interpreter frames really only identifiable through the marker being a > SMI, > > or is there a more reliable way to detect them? E.g. by marking a bit on the > > code object? > > The marker is not a SMI (it's a tagged JSFunction object, like other JavaScript > frames). It is identified by the fact that the return address always points to > the InterpreterEntryTrampoline builtin for interpreted frames. > > If you want to make it more robust I could do the same trick as the > ArgumentsAdapter (put a SMI marker in the context slot and push the context > separately. Would this be better? What was your plan for WASM frames? WASM frames will need to check the code kind, because the stack frames don't have markers.
On 2015/10/15 19:52:05, titzer wrote: > On 2015/10/15 19:47:47, rmcilroy wrote: > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc > > File src/frames.cc (right): > > > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 > > src/frames.cc:442: case Code::BUILTIN: > > On 2015/10/15 17:01:55, titzer wrote: > > > Are interpreter frames really only identifiable through the marker being a > > SMI, > > > or is there a more reliable way to detect them? E.g. by marking a bit on the > > > code object? > > > > The marker is not a SMI (it's a tagged JSFunction object, like other > JavaScript > > frames). It is identified by the fact that the return address always points > to > > the InterpreterEntryTrampoline builtin for interpreted frames. > > > > If you want to make it more robust I could do the same trick as the > > ArgumentsAdapter (put a SMI marker in the context slot and push the context > > separately. Would this be better? What was your plan for WASM frames? > > WASM frames will need to check the code kind, because the stack frames don't > have markers. That sounds pretty similar to what I'm doing here - checking the code kind is BUILTIN and return address points to the trampoline.
On 2015/10/15 20:40:06, rmcilroy wrote: > On 2015/10/15 19:52:05, titzer wrote: > > On 2015/10/15 19:47:47, rmcilroy wrote: > > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc > > > File src/frames.cc (right): > > > > > > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 > > > src/frames.cc:442: case Code::BUILTIN: > > > On 2015/10/15 17:01:55, titzer wrote: > > > > Are interpreter frames really only identifiable through the marker being a > > > SMI, > > > > or is there a more reliable way to detect them? E.g. by marking a bit on > the > > > > code object? > > > > > > The marker is not a SMI (it's a tagged JSFunction object, like other > > JavaScript > > > frames). It is identified by the fact that the return address always points > > to > > > the InterpreterEntryTrampoline builtin for interpreted frames. > > > > > > If you want to make it more robust I could do the same trick as the > > > ArgumentsAdapter (put a SMI marker in the context slot and push the context > > > separately. Would this be better? What was your plan for WASM frames? > > > > WASM frames will need to check the code kind, because the stack frames don't > > have markers. > > That sounds pretty similar to what I'm doing here - checking the code kind is > BUILTIN and return address points to the trampoline. Sure, but in frames.cc, it's just checking the marker is a SMI when computing the frame type.
On 2015/10/15 20:40:06, rmcilroy wrote: > On 2015/10/15 19:52:05, titzer wrote: > > On 2015/10/15 19:47:47, rmcilroy wrote: > > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc > > > File src/frames.cc (right): > > > > > > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 > > > src/frames.cc:442: case Code::BUILTIN: > > > On 2015/10/15 17:01:55, titzer wrote: > > > > Are interpreter frames really only identifiable through the marker being a > > > SMI, > > > > or is there a more reliable way to detect them? E.g. by marking a bit on > the > > > > code object? > > > > > > The marker is not a SMI (it's a tagged JSFunction object, like other > > JavaScript > > > frames). It is identified by the fact that the return address always points > > to > > > the InterpreterEntryTrampoline builtin for interpreted frames. > > > > > > If you want to make it more robust I could do the same trick as the > > > ArgumentsAdapter (put a SMI marker in the context slot and push the context > > > separately. Would this be better? What was your plan for WASM frames? > > > > WASM frames will need to check the code kind, because the stack frames don't > > have markers. > > That sounds pretty similar to what I'm doing here - checking the code kind is > BUILTIN and return address points to the trampoline. Sure, but in frames.cc, it's just checking the marker is a SMI when computing the frame type.
On 2015/10/16 02:33:40, titzer wrote: > On 2015/10/15 20:40:06, rmcilroy wrote: > > On 2015/10/15 19:52:05, titzer wrote: > > > On 2015/10/15 19:47:47, rmcilroy wrote: > > > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc > > > > File src/frames.cc (right): > > > > > > > > > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 > > > > src/frames.cc:442: case Code::BUILTIN: > > > > On 2015/10/15 17:01:55, titzer wrote: > > > > > Are interpreter frames really only identifiable through the marker being > a > > > > SMI, > > > > > or is there a more reliable way to detect them? E.g. by marking a bit on > > the > > > > > code object? > > > > > > > > The marker is not a SMI (it's a tagged JSFunction object, like other > > > JavaScript > > > > frames). It is identified by the fact that the return address always > points > > > to > > > > the InterpreterEntryTrampoline builtin for interpreted frames. > > > > > > > > If you want to make it more robust I could do the same trick as the > > > > ArgumentsAdapter (put a SMI marker in the context slot and push the > context > > > > separately. Would this be better? What was your plan for WASM frames? > > > > > > WASM frames will need to check the code kind, because the stack frames don't > > > have markers. > > > > That sounds pretty similar to what I'm doing here - checking the code kind is > > BUILTIN and return address points to the trampoline. > > Sure, but in frames.cc, it's just checking the marker is a SMI when computing > the frame type. I'm not sure where this discussion is going - is there any part of the current approach I've taken here you don't like? I'm open to suggestions, I'm just not sure what the concrete concerns you have are.
On 2015/10/16 10:50:02, rmcilroy wrote: > On 2015/10/16 02:33:40, titzer wrote: > > On 2015/10/15 20:40:06, rmcilroy wrote: > > > On 2015/10/15 19:52:05, titzer wrote: > > > > On 2015/10/15 19:47:47, rmcilroy wrote: > > > > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc > > > > > File src/frames.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 > > > > > src/frames.cc:442: case Code::BUILTIN: > > > > > On 2015/10/15 17:01:55, titzer wrote: > > > > > > Are interpreter frames really only identifiable through the marker > being > > a > > > > > SMI, > > > > > > or is there a more reliable way to detect them? E.g. by marking a bit > on > > > the > > > > > > code object? > > > > > > > > > > The marker is not a SMI (it's a tagged JSFunction object, like other > > > > JavaScript > > > > > frames). It is identified by the fact that the return address always > > points > > > > to > > > > > the InterpreterEntryTrampoline builtin for interpreted frames. > > > > > > > > > > If you want to make it more robust I could do the same trick as the > > > > > ArgumentsAdapter (put a SMI marker in the context slot and push the > > context > > > > > separately. Would this be better? What was your plan for WASM frames? > > > > > > > > WASM frames will need to check the code kind, because the stack frames > don't > > > > have markers. > > > > > > That sounds pretty similar to what I'm doing here - checking the code kind > is > > > BUILTIN and return address points to the trampoline. > > > > Sure, but in frames.cc, it's just checking the marker is a SMI when computing > > the frame type. > > I'm not sure where this discussion is going - is there any part of the current > approach I've taken here you don't like? I'm open to suggestions, I'm just not > sure what the concrete concerns you have are. I was just concerned about the DCHECK in frames.cc that it's the interpreter entry point is pretty weak. I'll lgtm this for now, but I'm wondering if it isn't going to be brittle and bite us.
> I was just concerned about the DCHECK in frames.cc that it's the interpreter > entry point is pretty weak. > > I'll lgtm this for now, but I'm wondering if it isn't going to be brittle and > bite us. Thanks. I'm hoping the DCHECK isn't brittle - INTERPRETED frames will always have a return address of the trampoline, but I'll keep an eye on this and rework it if we end up seeing brittleness.
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407513003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407513003/40001
Message was sent while issue was closed.
Committed patchset #1 (id:40001)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4b2fffae4c42f0e95333f1f3466c5d8ba7ee4585 Cr-Commit-Position: refs/heads/master@{#31342}
Message was sent while issue was closed.
On 2015/10/16 14:00:00, rmcilroy wrote: > > I was just concerned about the DCHECK in frames.cc that it's the interpreter > > entry point is pretty weak. > > > > I'll lgtm this for now, but I'm wondering if it isn't going to be brittle and > > bite us. > > Thanks. I'm hoping the DCHECK isn't brittle - INTERPRETED frames will always > have a return address of the trampoline, but I'll keep an eye on this and rework > it if we end up seeing brittleness. Ok, sgtm. I'll also be in here mucking things up for WebAssembly frames in not too long. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
