Profiler using measureme #317
Conversation
Razican
added
enhancement
|
@Razican @HalidOdat There's some great progress happening with this. |
I'm loving this. This should give nice insights on where do we spend our time. |
|
Is that flamegraph displayed in a Chrome devtools window? |
Yep! :) |
|
Ok, an SVG file loaded in devtools? |
Yes, the implementation is based in this: https://blog.rust-lang.org/inside-rust/2020/02/25/intro-rustc-self-profile.html |
jasonwilliams
force-pushed
the
profiler
branch
2 times, most recently
from
7e764b5
to
ef31daf
Compare
|
@Razican You asked about the realm profile image You should be able to load this into Chrome |
This looks like a clear improvement path. Could we also have internal metrics for each of the intrinsics creation? |
Yeah we can go as deep as we want really, its up to us what we want to trace |
In this case, I would add smaller steps in things that take too long, like this one. We can then review what we could do to improve this :) |
Benchmark for d526ee1Click to view benchmark
|
Benchmark for f8dfe95Click to view benchmark
|
|
latest gist shows each function being created |
Benchmark for 4927a91Click to view benchmark
|
Benchmark for ecb8770Click to view benchmark
|
Benchmark for be8e8f1Click to view benchmark
|
Benchmark for 5427d10Click to view benchmark
|
|
60% of time is spent in |
I thought as much, I tested with removing |
Benchmark for dd472feClick to view benchmark
|
That's interesting, do we have a clean way of refactoring this? |
The method that I used was only for testing, it not clean at all. I just extracted I think I'll try to do something more permanent in #419 |
There was a problem hiding this comment.
It looks very nice, I think we can use this to profile where Boa spends it's time, and it's already bringing improvements!
I will give tomorrow a look to the OnceCell usage, since I have some time. I think we can remove the unsafe code, and maybe remove the calls to drop().
| @@ -71,15 +74,23 @@ pub fn forward(engine: &mut Interpreter, src: &str) -> String { | |||
| /// The str is consumed and the state of the Interpreter is changed | |||
| /// Similar to `forward`, except the current value is returned instad of the string | |||
| /// If the interpreter fails parsing an error value is returned instead (error object) | |||
| #[allow(clippy::unit_arg, clippy::drop_copy)] | |||
There was a problem hiding this comment.
Just for my understanding, why is this needed here? Is the timer Copy or the profiler?
There was a problem hiding this comment.
error: calls to `std::mem::drop` with a value that implements `Copy`. Dropping a copy leaves the original intact.
--> boa\src\lib.rs:90:5
|
90 | drop(main_timer);
| ^^^^^^^^^^^^^^^^
|
note: the lint level is defined here
--> boa\src\lib.rs:9:5
|
9 | clippy::all,
| ^^^^^^^^^^^
= note: `#[deny(clippy::drop_copy)]` implied by `#[deny(clippy::all)]`
note: argument has type ()
--> boa\src\lib.rs:90:10
|
90 | drop(main_timer);
| ^^^^^^^^^^
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#drop_copy
error: aborting due to previous error
error: could not compile `Boa`.
So it looks like () implements Copy and when we don't compile the profiler, we're just dropping () which triggers this linting warning.
There was a problem hiding this comment.
This could be solved by only calling drop() with the feature active. We can add an artificial scope here and add the #[cfg] flag.
* init() is not needed * Re-order doc
Benchmark for 77bfd02Click to view benchmark
|
Benchmark for 36009ddClick to view benchmark
|
| pub fn default() -> BoaProfiler { | ||
| let profiler = Profiler::new(Path::new("./my_trace")).unwrap(); | ||
| BoaProfiler { profiler } | ||
| } |
There was a problem hiding this comment.
We should implement the Default trait, and any function returning BoaProfiler in BoaProfiler should return Self, in case we change its name in the future, to make it easier.
| pub fn drop(&self) { | ||
| // In order to drop the INSTANCE we need to get ownership of it, which isn't possible on a static unless you make it a mutable static | ||
| // mutating statics is unsafe, so we need to wrap it as so. | ||
| // This is actually safe though because init and drop are only called at the beginning and end of the application | ||
| unsafe { | ||
| INSTANCE | ||
| .take() | ||
| .expect("Could not take back profiler instance"); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm afraid this is not safe. If we call forward() more than once (which we sometimes do), this would be undefined behaviour.
I think the best solution is not to use a static global here. We could use a Mutex to avoid this, but I think this would reduce performance a lot. I think the best option is to conditionally have a BoaProfiler in the Lexer, Parser and Interpreter.
In the future, we could have a global Boa object with this field. In any case, I think the three of them should have a start_event() that would be no-op if the feature is not active. And all event starts should call to the Lexer, Parser or Interpreter.
Codecov Report
@@ Coverage Diff @@
## master #317 +/- ##
==========================================
- Coverage 66.64% 66.09% -0.55%
==========================================
Files 124 128 +4
Lines 8852 8992 +140
==========================================
+ Hits 5899 5943 +44
- Misses 2953 3049 +96
Continue to review full report at Codecov.
|
Benchmark for 11baf79Click to view benchmark
|
fixes jasonwilliams#296