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 heap snapsnot memory tracking for Event, EventTarget #1605

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 1, 2024

Expands heap tracking memory tracking for handful of API types

Includes the context global in the heap tracking trace. originally had some extra stuff in here for special-case handling of the context global in order for it to appear in the snapshot a certain way but it added way more complexity than it was worth. The non-special case handling is sufficient.

Refines a few bits and pieces in the memory tracker def

@jasnell jasnell requested review from a team as code owners February 1, 2024 19:08
@jasnell jasnell force-pushed the jsnell/basics-memory-tracker branch 2 times, most recently from 7674f3e to dbd624a Compare February 2, 2024 02:24
@jasnell jasnell changed the title Implement heap snapsnot memory tracking for Event, EventTarget Implement heap snapsnot memory tracking for Event, EventTarget, Global context Feb 2, 2024
@jasnell jasnell force-pushed the jsnell/basics-memory-tracker branch 2 times, most recently from dbe4e03 to d84952a Compare February 5, 2024 16:32
@jasnell jasnell requested review from mikea, hoodmane and fhanau February 5, 2024 16:35
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.

I don't fully understand the changes to memory.h. Everything else looks reasonable to me. One question: what is the strategy to keep these visitForMemoryInfo implementations up to date as the code changes? I guess it's a similar business to having to remember to tell the garbage collector about fields you add.

src/workerd/api/actor-state-test.c++ Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Feb 6, 2024

I guess it's a similar business to having to remember to tell the garbage collector about fields you add.

Yep, we just have to be disciplined to keep it up to date. Fortunately there's less impact here if we get this wrong than with visitForGc()

@jasnell jasnell force-pushed the jsnell/basics-memory-tracker branch from d84952a to bfc7f65 Compare February 6, 2024 03:51
@jasnell
Copy link
Member Author

jasnell commented Feb 6, 2024

There are still a few things I want to look at here, and I might end up pulling the further simplifications made to memory.h in one of the other prs into this... going to move this back to draft so it doesn't get merged before then

@jasnell jasnell marked this pull request as draft February 6, 2024 03:55
@jasnell jasnell force-pushed the jsnell/basics-memory-tracker branch from bfc7f65 to 736599d Compare February 6, 2024 07:37
@jasnell jasnell marked this pull request as ready for review February 6, 2024 07:38
@jasnell jasnell changed the title Implement heap snapsnot memory tracking for Event, EventTarget, Global context Implement heap snapsnot memory tracking for Event, EventTarget Feb 6, 2024
@jasnell jasnell force-pushed the jsnell/basics-memory-tracker branch from 736599d to fd52869 Compare February 6, 2024 14:59
@jasnell jasnell merged commit 99b1fd2 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