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 snapshot memorytracker for http types #1631

Merged
merged 1 commit into from
Feb 7, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions src/workerd/api/http.h
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,12 @@ class Headers: public jsg::Object {
});
}

void visitForMemoryInfo(jsg::MemoryTracker& tracker) const {
for (const auto& entry : headers) {
tracker.trackField(entry.first, entry.second);
}
}

private:
struct Header {
jsg::ByteString key; // lower-cased name
Expand All @@ -187,6 +193,14 @@ class Headers: public jsg::Object {
: key(kj::mv(key)), name(kj::mv(name)), values(1) {
values.add(kj::mv(value));
}

JSG_MEMORY_INFO(Header) {
tracker.trackField("key", key);
tracker.trackField("name", name);
for (const auto& value : values) {
tracker.trackField(nullptr, value);
}
}
};

Guard guard;
Expand Down Expand Up @@ -238,6 +252,9 @@ class Body: public jsg::Object {
struct RefcountedBytes final: public kj::Refcounted {
kj::Array<kj::byte> bytes;
RefcountedBytes(kj::Array<kj::byte>&& bytes): bytes(kj::mv(bytes)) {}
JSG_MEMORY_INFO(RefcountedBytes) {
tracker.trackFieldWithSize("bytes", bytes.size());
}
};

// The Fetch spec calls this type the body's "source", even though it really is a buffer. I end
Expand Down Expand Up @@ -278,11 +295,26 @@ class Body: public jsg::Object {
view(ownBytes.get<jsg::Ref<Blob>>()->getData()) {}

Buffer clone(jsg::Lock& js);

JSG_MEMORY_INFO(Buffer) {
KJ_SWITCH_ONEOF(ownBytes) {
KJ_CASE_ONEOF(bytes, kj::Own<RefcountedBytes>) {
tracker.trackField("bytes", bytes);
}
KJ_CASE_ONEOF(blob, jsg::Ref<Blob>) {
tracker.trackField("blob", blob);
}
}
}
};

struct Impl {
jsg::Ref<ReadableStream> stream;
kj::Maybe<Buffer> buffer;
JSG_MEMORY_INFO(Impl) {
tracker.trackField("stream", stream);
tracker.trackField("buffer", buffer);
}
};

struct ExtractedBody {
Expand Down Expand Up @@ -346,6 +378,10 @@ class Body: public jsg::Object {
// Allow JSON body type to be specified
}

void visitForMemoryInfo(jsg::MemoryTracker& tracker) const {
tracker.trackField("impl", impl);
}

protected:
// Helper to implement Request/Response::clone().
kj::Maybe<ExtractedBody> clone(jsg::Lock& js);
Expand Down Expand Up @@ -843,6 +879,15 @@ class Request: public Body {
}
}

void visitForMemoryInfo(jsg::MemoryTracker& tracker) const {
tracker.trackField("url", url);
tracker.trackField("headers", headers);
tracker.trackField("fetcher", fetcher);
tracker.trackField("signal", signal);
tracker.trackField("thisSignal", thisSignal);
tracker.trackField("cf", cf);
}

private:
kj::HttpMethod method;
kj::String url;
Expand Down Expand Up @@ -1020,6 +1065,17 @@ class Response: public Body {
// Use `BodyInit` and `ResponseInit` type aliases in constructor instead of inlining
}

void visitForMemoryInfo(jsg::MemoryTracker& tracker) const {
tracker.trackField("statusText", statusText);
tracker.trackField("headers", headers);
tracker.trackField("webSocket", webSocket);
tracker.trackField("cf", cf);
for (const auto& url : urlList) {
tracker.trackField("urlList", url);
}
tracker.trackField("asyncContext", asyncContext);
}

private:
int statusCode;
kj::String statusText;
Expand Down Expand Up @@ -1083,6 +1139,13 @@ class FetchEvent: public ExtendableEvent {
JSG_METHOD(passThroughOnException);
}

void visitForMemoryInfo(jsg::MemoryTracker& tracker) const {
tracker.trackField("request", request);
KJ_IF_SOME(respondWithCalled, state.tryGet<RespondWithCalled>()) {
tracker.trackField("promise", respondWithCalled.promise);
}
}

private:
jsg::Ref<Request> request;

Expand All @@ -1096,6 +1159,9 @@ class FetchEvent: public ExtendableEvent {

void visitForGc(jsg::GcVisitor& visitor) {
visitor.visit(request);
KJ_IF_SOME(respondWithCalled, state.tryGet<RespondWithCalled>()) {
visitor.visit(respondWithCalled.promise);
}
Comment on lines +1162 to +1164
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this fix a memory leak?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might. If the promise is left dangling and any closures attached to it happen to create a cycle then yes, it could have leaked. A big part of the reason I'm adding this memorytracker mechanism is to help find anything potentially like this.

}
};

Expand Down
Loading