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

Adds performance annotations using the new Hatchet annotation interface #195

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ilumsden
Copy link
Collaborator

This PR adds performance measurement annotations to Thicket using the APIs added to Hatchet in LLNL/hatchet#142.

@ilumsden ilumsden added area-utils Issues and PRs related to Thicket's internal utilities and helpers priority-normal Normal priority issues and PRs status-ready-for-review This PR is ready to be reviewed by assigned reviewers type-feature Requests for new features or PRs which implement new features labels Jul 14, 2024
@ilumsden ilumsden self-assigned this Jul 14, 2024
Copy link
Collaborator

@dyokelson dyokelson left a comment

Choose a reason for hiding this comment

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

@ilumsden Can you remind me - is there anything important about the order of the decorators or are they applied independently? Just wondering what the outcome will be for some functions with the cache_stats_op and perf annotations.

Otherwise looks good, since this is just annotations and the functionality is in hatchet.

Copy link
Collaborator

@michaelmckinsey1 michaelmckinsey1 left a comment

Choose a reason for hiding this comment

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

A couple of comments:

  1. Is it possible to add metadata with the pycaliper interface?
  2. I think the amount of annotations is causing high overhead. See following:

Time to create Thicket with X amount of files.

files pycaliper develop
1,000 36s 8s
10,000 15m36s 1m26s

I notice Calls for Node.__eq__ is 96,174,300 for the thicket with 10,000 files. By removing all of the node annotations in Hatchet for pycaliper time goes from 15m36s to 3m. So I think we may have to be more selective with what we annotate or maybe provide some kind of different levels for profiling.

@ilumsden
Copy link
Collaborator Author

Thanks for taking a look @michaelmckinsey1. That timing info is very interesting. I definitely agree that the annotations need to be paired down given the timing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-utils Issues and PRs related to Thicket's internal utilities and helpers priority-normal Normal priority issues and PRs status-ready-for-review This PR is ready to be reviewed by assigned reviewers type-feature Requests for new features or PRs which implement new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants