Add `Vm` trace functionality and add trace to `boa_wasm` #3227

nekevss · 2023-08-19T16:43:02Z

This is the branch that was used for the web-debugger concept.

This branch is technically ready to be merged, but thought I'd post it as a draft for now to get feedback as it has some API changes along with quite a few changes to the Vm trace overall.

It changes the following:

Search and handle the compiled bytecode trace prior to instruction execution and trace.
Creates a BoaJs object and an evaluate_with_debug_hooks function in boa_wasm
Creates a VmTrace (not the biggest fan of the name here if anyone has other ideas) for handling tracing and allowing users to define handle trace themselves via "registering" a function (defaults to println).

For reference, the standard trace output would now print like the below.

----------------------Compiled Output: '<main>'-----------------------
Location  Count    Handler    Opcode                     Operands

000000    0000      none      GetName                    0000: 'greet'
000005    0001      none      PushUndefined              
000006    0002      none      Swap                       
000007    0003      none      PushLiteral                0
000012    0004      none      Call                       1
000017    0005      none      SetReturnValue             
000018    0006      none      Return                     

Literals:
    0000: <string> "World"

Bindings:
    0000: greet

Functions:
    0000: name: 'greet' (length: 1)

Handlers:
    <empty>


-----------------------Compiled Output: 'greet'-----------------------
Location  Count    Handler    Opcode                     Operands

000000    0000      none      PutLexicalValue            0000: 'targetName'
000005    0001      none      RestParameterPop           
000006    0002      none      PushLiteral                0
000011    0003      none      GetName                    0000: 'targetName'
000016    0004      none      Add                        
000017    0005      none      PushLiteral                1
000022    0006      none      Add                        
000023    0007      none      SetReturnValue             
000024    0008      none      Return                     
000025    0009      none      Return                     

Literals:
    0000: <string> "Hello, "
    0001: <string> "!"

Bindings:
    0000: targetName

Functions:
    <empty>

Handlers:
    <empty>

------------------------------------ Call Frame -- <main> ------------------------------------
Time          Opcode                     Operands                   Stack

14╬╝s          GetName                    0000: 'greet'              [ [function] ]
0╬╝s           PushUndefined                                         [ undefined, [function] ]
0╬╝s           Swap                                                  [ [function], undefined ]
1╬╝s           PushLiteral                0                          [ "World", [function], undefined ]
------------------------------------ Call Frame -- greet -------------------------------------
0╬╝s           PutLexicalValue            0000: 'targetName'         [ ]
1╬╝s           RestParameterPop                                      [ ]
0╬╝s           PushLiteral                0                          [ "Hello, " ]
1╬╝s           GetName                    0000: 'targetName'         [ "World", "Hello, " ]
5╬╝s           Add                                                   [ "Hello, World" ]
0╬╝s           PushLiteral                1                          [ "!", "Hello, World" ]
1╬╝s           Add                                                   [ "Hello, World!" ]
0╬╝s           SetReturnValue                                        [ ]
0╬╝s           Return                                                [ ]
-------------------------- Call Frame -- <Exiting greet via Return> --------------------------
140╬╝s         Call                       1                          [ "Hello, World!" ]
0╬╝s           SetReturnValue                                        [ ]
0╬╝s           Return                                                [ ]
------------------------- Call Frame -- <Exiting <main> via Return> --------------------------
"Hello, World!"

Let me know what you think 😄

github-actions · 2023-08-19T16:51:18Z

Test262 conformance changes

Test result	main count	PR count	difference
Total	50,731	50,731	0
Passed	42,973	42,973	0
Ignored	1,395	1,395	0
Failed	6,363	6,363	0
Panics	18	18	0
Conformance	84.71%	84.71%	0.00%

jedel1043 · 2023-08-19T20:04:32Z

Seeing this, I think it would be worth to create a Tracer trait instead of passing closures to handle tracing. Would also make it easier to send the output to files, loggers or other interesting usages.

nekevss · 2023-08-19T20:35:46Z

Would also make it easier to send the output to files, loggers or other interesting usages.

Kind of what I was going for.

I was initially thinking of how to plug the trace output into a tui out of curiosity which led me to the wasm. I don't think it should be too crazy to switch it over to being trait based. 😄

codecov · 2023-08-20T01:28:44Z

Codecov Report

Attention: Patch coverage is 10.10638% with 169 lines in your changes are missing coverage. Please review.

Project coverage is 47.21%. Comparing base (6ddc2b4) to head (606f48e).
Report is 141 commits behind head on main.

❗ Current head 606f48e differs from pull request most recent head 94045e1. Consider uploading reports for the commit 94045e1 to get more accurate results

Files	Patch %	Lines
core/engine/src/vm/trace.rs	13.92%	68 Missing ⚠️
ffi/wasm/src/lib.rs	0.00%	64 Missing ⚠️
cli/src/debug/function.rs	0.00%	15 Missing ⚠️
core/engine/src/vm/code_block.rs	0.00%	10 Missing ⚠️
core/engine/src/vm/mod.rs	57.14%	6 Missing ⚠️
core/engine/src/context/mod.rs	0.00%	4 Missing ⚠️
cli/src/main.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3227      +/-   ##
==========================================
- Coverage   47.24%   47.21%   -0.03%     
==========================================
  Files         476      476              
  Lines       46892    46084     -808     
==========================================
- Hits        22154    21760     -394     
+ Misses      24738    24324     -414

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nekevss · 2023-08-20T01:58:06Z

Switched the custom closures bits over to a trait.

It may be possible to go even further with the trait than currently implemented and selectively emit Vm state rather than pre-formatted messages. But I'm not exactly sure how much access we should provided, so I kept the current output as is for now.

jedel1043 · 2023-11-29T18:50:42Z

@HalidOdat and @jedel1043 will review this to see if we want to expose the instructions or if this is good enough.

jedel1043

Really nice API! This just needs a rebase, since it's still using the old std::time::Instant. I also have some suggestions and a small question.

boa_engine/src/vm/code_block.rs

boa_engine/src/vm/mod.rs

boa_engine/src/vm/trace.rs

jedel1043 · 2023-12-04T00:20:28Z

boa_engine/src/vm/trace.rs

+        if self.is_full_trace() {
+            self.trace_compiled_bytecode(vm, interner);
+            self.call_frame_header(vm);
+        } else if self.is_partial_trace() && vm.frame().code_block().traceable() {
+            if !vm.frame().code_block().frame_traced() {
+                self.trace_current_bytecode(vm, interner);
+                vm.frame().code_block().set_frame_traced(true);
+            }
+            self.call_frame_header(vm);
+            self.activate();
+        } else {
+            self.call_frame_header(vm);
+        }


Hmm, I think we can reduce the complexity of this by adding a Trace::should_trace(code: &Gc<CodeBlock>) -> ShouldTrace method with an enum defined by:

enum ShouldTrace { No, OnlyInstructions, Full }

Tracers can carry a traceable list of Gc<Codeblock> to determine if a code should be traced, and also a first_trace list to determine which codeblocks haven't been traced so that it displays their bytecode first.

Still need to work on this portion. The rest is rebased, so it shouldn't take too long 😄

EDIT: Actually I was looking at the above, and I'm not really sure why that was so convoluted. I cleaned up the logic a bit, so it may be better.

Added an enum (went with TraceAction over ShouldTrace), but now some of that logic is loaded into should_trace. Let me know what you think.

Really nice! Though, I think I didn't explain myself good enough. The goal of adding that enum was so that we can move the "should be traced partially or fully" logic from VmTracer to the tracers itself, by adding a Tracer::should_trace(&self, code: Gc<CodeBlock>) -> TraceAction method. Then, implementors would be the ones taking care of choosing when to just print the instructions vs the compiled bytecode.

Should we be exposing that much of the Vm internals to Tracer? Part of the current implementation is to only expose preformatted strings in a bit more of a "controlled environment" so to speak, which included handling the logic aspect.

I'm pretty sure we already expose CodeBlock publicly, and users cannot do much with it aside from getting its address and name, so I think it should be fine.

I'm playing around with the update, and it's nicer in some places, but feels a lot more convoluted in others. I'm also not entirely sure on how to calling multiple $boa.function.traceable() might work as calling the function would remove the previous Tracer, which may cause problems on generators and async functions.

I'd say that we should try to find an architecture that allows us to always use the same Tracer, mostly for simplicity.

HalidOdat

Sorry for the late review.

Besides jedel1043's comments, this looks good me! :)

HalidOdat

Did another review, just some small nitpicks that don't block merging. The rest looks good to me! :)

core/engine/src/vm/trace.rs

core/engine/src/vm/code_block.rs

nekevss · 2024-01-20T01:13:44Z

Made some changes to have VmTrace have a Vec<Box<dyn Tracer>> and moved should_trace to Tracer. Let me know what you think!

HalidOdat · 2024-02-04T06:44:01Z

Looks good to me! Just needs a rebase :)

nekevss · 2024-04-19T01:32:46Z

core/engine/src/vm/mod.rs

@@ -76,7 +83,7 @@ pub struct Vm {
    pub(crate) realm: Realm,

    #[cfg(feature = "trace")]
-    pub(crate) trace: bool,
+    pub(crate) trace: VmTrace,


A thought that I've had over the last day or so is whether it would be worthwhile redesigning so that the Trace is a trait on Vm.

struct Vm<T: ExecutionTrace=()> { ... trace: T }

The only issue with this that I haven't totally thought through is that I'm not sure we'd be able to feature flag the fields (all though we could probably still feature flag the method calls)

I think the current design is fine. What would be the benefit of having it as a trait?

Mostly I'm not sure if I'm totally in love with the Vec<Box<dyn Tracer>> on VmTrace. It works and improves the current functionality, but I'm mostly just thinking whether there may be a bit of a better design 😕 That being said, I'm not sure a theoretically better design out weighs the functionality here.

Edit: Using a trait would allow the trace to be defined at compile time essentially.

Imo the Vec<Box<dyn Tracer>> is totally fine for this use case.

Using a trait would allow the trace to be defined at compile time essentially.

I'm not sure if that is a benefit. The upside would be reducing dynamic dispatch, but if you enable tracing, you probably do not care about that last bit of performance.
Being able to change the tracing at runtime also seems like a nice feature right?

It is a nice feature! And you're right, if someone enables tracing then they are probably not concerned about performance. But if it's switched to a trait that doesn't mean that there couldn't be an implementation of the trait that can be updated at runtime. And if we were to update this to something without dynamic dispatching, then we even provide an impl of the trait that could be updated at runtime.

The primary difference would be that it would be a user deciding to use dynamic dispatching over Boa locking the user into using it.

I would say the current implementation is good engough. If there are shortcomings we could change it. I would prefer that to adding a generic to the Vm struct. But if you think it is significantly better, go for it :)

I do think we could offer "combinators" for tracers. Instead of having Vec<Box<dyn Tracer>>, why not something like Box<dyn Tracer>, where we impl<T: Tracer, U: Tracer> Tracer for (T, U)?

😕 Maybe.

There's probably a better option with how Tracer is defined too. I was playing around with a new design and have something like:

pub trait Tracer: std::fmt::Debug { fn trace_instruction(&self, ctx: &TraceDecisionCtx<'_>) -> InstructionTraceAction; fn trace_bytecode(&self, ctx: &TraceDecisionCtx<'_>) -> BytecodeTraceAction; fn emit(&self, event: &TraceEvent<'_>); }

Where TraceEvent is something like:

pub enum TraceEvent<'a> { NewCallFrame(&'a NewFrameEvent<'a>), ExitCallFrame(&'a ExitFrameEvent<'a>), Instruction(&'a InstructionTrace), ByteCode(&'a BytecodeTraceEvent), }

But that design still has VmTrace using a Vec.

raskad

Just found some leftover comments / typos.

core/engine/src/context/mod.rs

core/engine/src/vm/code_block.rs

raskad · 2024-04-19T14:51:24Z

core/engine/src/vm/mod.rs

@@ -76,7 +83,7 @@ pub struct Vm {
    pub(crate) realm: Realm,

    #[cfg(feature = "trace")]
-    pub(crate) trace: bool,
+    pub(crate) trace: VmTrace,


I think the current design is fine. What would be the benefit of having it as a trait?

nekevss force-pushed the wasm-debugger branch from d40ba61 to 58ca1b2 Compare September 23, 2023 16:39

nekevss marked this pull request as ready for review September 23, 2023 17:04

nekevss requested a review from a team September 23, 2023 17:04

jedel1043 added API waiting-on-review Waiting on reviews from the maintainers labels Nov 29, 2023

jedel1043 assigned HalidOdat and jedel1043 Nov 29, 2023

jedel1043 reviewed Dec 4, 2023

View reviewed changes

boa_engine/src/vm/code_block.rs Outdated Show resolved Hide resolved

boa_engine/src/vm/mod.rs Outdated Show resolved Hide resolved

boa_engine/src/vm/trace.rs Outdated Show resolved Hide resolved

jedel1043 reviewed Dec 4, 2023

View reviewed changes

HalidOdat approved these changes Dec 6, 2023

View reviewed changes

HalidOdat added this to the v0.18.0 milestone Dec 6, 2023

nekevss force-pushed the wasm-debugger branch from 58ca1b2 to 89344fd Compare December 7, 2023 02:08

HalidOdat approved these changes Dec 8, 2023

View reviewed changes

core/engine/src/vm/trace.rs Outdated Show resolved Hide resolved

core/engine/src/vm/code_block.rs Outdated Show resolved Hide resolved

nekevss force-pushed the wasm-debugger branch from abacf7f to 80d8ac0 Compare January 20, 2024 00:21

nekevss requested a review from jedel1043 January 20, 2024 01:11

HalidOdat approved these changes Feb 4, 2024

View reviewed changes

nekevss force-pushed the wasm-debugger branch from 606f48e to 6fa254b Compare February 4, 2024 14:15

nekevss mentioned this pull request Apr 12, 2024

Improvements on conformance page boa-dev/boa-dev.github.io#148

Open

7 tasks

nekevss added 4 commits April 18, 2024 18:53

Squash wasm-debugger to one commit

0baa009

Post-rebase fmt

a1900c8

Clean up logic for clarity

2ae0204

Review feedback

e0e64d2

nekevss added 6 commits April 18, 2024 18:54

Add tracer name and fix/clean some logic

a07ba2a

Fix TraceAction import

8099c04

cargo fmt all

39f973e

Redesign for Vec<Box<dyn Tracer>>

ed1cf6c

cargo fmt and clippy

b20bb5b

cargo fmt

1b95f81

nekevss force-pushed the wasm-debugger branch from b42de24 to 1b95f81 Compare April 18, 2024 23:03

nekevss commented Apr 19, 2024

View reviewed changes

raskad reviewed Apr 19, 2024

View reviewed changes

Remove comment and fix duplicate main trace

94045e1

nekevss force-pushed the wasm-debugger branch from ab258d1 to 94045e1 Compare April 20, 2024 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Vm` trace functionality and add trace to `boa_wasm` #3227

Add `Vm` trace functionality and add trace to `boa_wasm` #3227

nekevss commented Aug 19, 2023

github-actions bot commented Aug 19, 2023 •

edited

jedel1043 commented Aug 19, 2023 •

edited

nekevss commented Aug 19, 2023

codecov bot commented Aug 20, 2023 •

edited

nekevss commented Aug 20, 2023

jedel1043 commented Nov 29, 2023

jedel1043 left a comment

jedel1043 Dec 4, 2023 •

edited

nekevss Dec 7, 2023 •

edited

nekevss Dec 8, 2023

jedel1043 Dec 9, 2023

nekevss Dec 9, 2023

jedel1043 Dec 9, 2023 •

edited

nekevss Dec 9, 2023

jedel1043 Dec 22, 2023

HalidOdat left a comment

HalidOdat left a comment

nekevss commented Jan 20, 2024

HalidOdat commented Feb 4, 2024

nekevss Apr 19, 2024

raskad Apr 19, 2024

nekevss Apr 19, 2024 •

edited

raskad Apr 19, 2024

nekevss Apr 19, 2024 •

edited

raskad Apr 19, 2024

jedel1043 May 7, 2024 •

edited

nekevss May 11, 2024

raskad left a comment

raskad Apr 19, 2024

Add Vm trace functionality and add trace to boa_wasm #3227

Are you sure you want to change the base?

Add Vm trace functionality and add trace to boa_wasm #3227

Conversation

nekevss commented Aug 19, 2023

github-actions bot commented Aug 19, 2023 • edited

Test262 conformance changes

jedel1043 commented Aug 19, 2023 • edited

nekevss commented Aug 19, 2023

codecov bot commented Aug 20, 2023 • edited

Codecov Report

nekevss commented Aug 20, 2023

jedel1043 commented Nov 29, 2023

jedel1043 left a comment

Choose a reason for hiding this comment

jedel1043 Dec 4, 2023 • edited

Choose a reason for hiding this comment

nekevss Dec 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Dec 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HalidOdat left a comment

Choose a reason for hiding this comment

HalidOdat left a comment

Choose a reason for hiding this comment

nekevss commented Jan 20, 2024

HalidOdat commented Feb 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nekevss Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nekevss Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raskad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `Vm` trace functionality and add trace to `boa_wasm` #3227

Add `Vm` trace functionality and add trace to `boa_wasm` #3227

github-actions bot commented Aug 19, 2023 •

edited

jedel1043 commented Aug 19, 2023 •

edited

codecov bot commented Aug 20, 2023 •

edited

jedel1043 Dec 4, 2023 •

edited

nekevss Dec 7, 2023 •

edited

jedel1043 Dec 9, 2023 •

edited

nekevss Apr 19, 2024 •

edited

nekevss Apr 19, 2024 •

edited

jedel1043 May 7, 2024 •

edited