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

Implement memory tracking for Blob/File/Streams #1608

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 2, 2024

No description provided.

@jasnell jasnell changed the title Implement memory tracking for Blob/File Implement memory tracking for Blob/File/Streams Feb 2, 2024
@jasnell jasnell force-pushed the jsnell/more-meory-tracker branch 2 times, most recently from a9982b1 to 94da874 Compare February 2, 2024 04:19
@jasnell jasnell force-pushed the jsnell/basics-memory-tracker branch from dbd624a to dbe4e03 Compare February 5, 2024 03:18
@jasnell jasnell force-pushed the jsnell/basics-memory-tracker branch from dbe4e03 to d84952a Compare February 5, 2024 16:32
@jasnell jasnell force-pushed the jsnell/basics-memory-tracker branch 3 times, most recently from 736599d to fd52869 Compare February 6, 2024 14:59
@jasnell jasnell changed the base branch from jsnell/basics-memory-tracker to main February 6, 2024 16:02
@jasnell jasnell marked this pull request as ready for review February 6, 2024 16:04
@jasnell jasnell requested review from a team as code owners February 6, 2024 16:04
Copy link
Contributor

@hoodmane hoodmane 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 to me.

Comment on lines +64 to +71
KJ_SWITCH_ONEOF(ownData) {
KJ_CASE_ONEOF(data, kj::Array<byte>) {
tracker.trackField("ownData", data);
}
KJ_CASE_ONEOF(data, jsg::Ref<Blob>) {
tracker.trackField("ownData", data);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be convenient if there were a way to call trackField with a kj::OneOf<S, T, ...> where there is an impl for trackField for each of them. Perhaps it's not worth the hassle to save a little bit of boilerplate.

But maybe you could make a concept Trackable with T::trackField(tracker, name, value) and then define tracker.trackField<Trackable T> to call T::trackField. Then maybe you can define OneOf<Trackable S, Trackable T, Trackable U>::trackField or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd considered adding that but I think I prefer separating it out like this in order to give a bit more control over how things are tracked. In some cases, it won't need to be tracked at all, in other cases it make use trackField(...) for one and trackFieldWithSize(...) for others.

@jasnell jasnell merged commit ec3a570 into main Feb 6, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants