Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Date with perf-hooks for time measurement #649

Merged
merged 7 commits into from
Feb 21, 2025
Merged

Conversation

TungstnBallon
Copy link
Contributor

Currently, Jayvee uses Date to measure the duration of hooks, blocks and pipelines.

With this PR, the Jayvee interpreter now sets "performance marks" at relevant points during execution (before/after blocks/pipelines) and then measures the time between these marks. For now, marks are named with the following schema, with the goal of "encoding" the mark's location in the name:

`${pipeline name}::${block name}::${preBlockHook or BlockExecution or postBlockHook}::{start or end}`

This isn't final though, and I other ideas are welcome.

IMO this is a better approach than using the standard Jayvee hooks. Quoting #636 (comment):

As of now, the hooks' execution order is not sophisticated enough to log execution duration. Currently generic hooks are executed before block specific hooks in the order they are added. Considering #637, the hook logging execution duration would have to be executed after all other hooks, which cannot be guaranteed at this point.

I hope, that performance hooks can be used to implement a benchmarking tool using PerformanceObserver. That need's further testing though, so this PR is a draft for now.

@TungstnBallon TungstnBallon requested a review from joluj February 20, 2025 10:42
Copy link
Contributor

@joluj joluj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! What's still missing?

@TungstnBallon
Copy link
Contributor Author

Looks good! What's still missing?

I wanted to verify that this can actually be used to create a benchmarking tool (see #650).

@TungstnBallon TungstnBallon marked this pull request as ready for review February 20, 2025 17:10
@TungstnBallon TungstnBallon requested a review from joluj February 20, 2025 17:10
Copy link
Contributor

@joluj joluj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

Could you add a bit more documentation, especially to functions exported from the lib.

@TungstnBallon TungstnBallon requested a review from joluj February 21, 2025 09:29
Copy link
Contributor

@joluj joluj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thank you!

@TungstnBallon TungstnBallon merged commit e6a952e into main Feb 21, 2025
3 checks passed
@TungstnBallon TungstnBallon deleted the perf_hooks branch February 21, 2025 11:10
@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants