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.

Sign up for GitHub

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

Jump to bottom

Adds APIs to jsg::Lock to perform manual external memory accounting. #2494

Merged
merged 1 commit into from
Aug 9, 2024
Merged

Adds APIs to jsg::Lock to perform manual external memory accounting. #2494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions src/workerd/jsg/jsg.c++
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,29 @@ JsSymbol Lock::symbolAsyncDispose() {
return IsolateBase::from(v8Isolate).getSymbolAsyncDispose();
}

void Lock::adjustExternalMemory(ssize_t amount) {
anonrig marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I think this API is error-prone: it's too easy to adjust the memory up and forget to adjust it back down later.

Could we create an RAII-based API instead? Have this method return some sort of an object whose destructor adjusts the memory back down. The object could also have a method to modify the amount of memory it's representing, for cases where we're tracking memory for a data structure that changes over time.

I think this object can just be a trivial wrapper around an integer; no allocation required. It can have a move constructor which zeros out the old value.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I missed that this PR actually introduces an RAII API as well.

I think we should only offer the RAII API. We can extend it to work well even when the memory allocation changes over time, using the technique I suggested. Also can avoid the need for a heap allocation.

v8Isolate->AdjustAmountOfExternalAllocatedMemory(static_cast<int64_t>(amount));
}

namespace {
struct ExternalMemoryAdjuster {
int64_t size;
v8::Isolate* isolate;
ExternalMemoryAdjuster(v8::Isolate* isolate, size_t size)
: size(static_cast<int64_t>(size)), isolate(isolate) {
isolate->AdjustAmountOfExternalAllocatedMemory(size);
}

~ExternalMemoryAdjuster() noexcept(false) {
isolate->AdjustAmountOfExternalAllocatedMemory(-size);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Review note: Obviously this can be a problem if the kj::Own<void> for this is held beyond the lifespan of the isolate. Should we include more protections against that by making the isolate effectively a weak ref rather than a bare pointer? Should we forgo this entirely in favor of just having the manual adjustExternalMemory(...) call?

The key use case for this would be to allow for something like..

auto ary = kj::heapArray<int>(10).attach(js.getExternalMemoryAdjuster(10 * sizeof(int)));

Such that the external memory will be automatically adjusted in the isolate when the array is freed. This, of course, is only really appropriate if the array does not get bound to an ArrayBuffer at any point, which would cause the data to be double accounted.

};
} // namespace

kj::Own<void> Lock::getExternalMemoryAdjuster(size_t amount) {
return kj::heap<ExternalMemoryAdjuster>(v8Isolate, amount);
}

Name::Name(kj::String string)
: hash(kj::hashCode(string)),
inner(kj::mv(string)) {}
Expand Down
14 changes: 14 additions & 0 deletions src/workerd/jsg/jsg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2085,6 +2085,20 @@ class Lock {
return *reinterpret_cast<Lock*>(v8Isolate->GetData(2));
}

// Manually make adjustments to the amount of external memory reported to V8.
// This is useful when we have a large amount of external memory allocated that
// typically would not be visible to v8's memory tracking. In the future we hope
// to make most memory accounting automatic, making most direct uses of this
// API unnecessary but there will always be times when manual adjustments are
// necessary.
void adjustExternalMemory(ssize_t amount);

// Reports amount of external memory to be manually attributed to the isolate.
// When the returned kj::Own<void> is dropped, the amount will be subtracted
// from the isolate's external memory accounting. It is important that these
// be held onto only during the lifetime of the isolate.
jasnell marked this conversation as resolved.
Show resolved Hide resolved
kj::Own<void> getExternalMemoryAdjuster(size_t amount);

Value parseJson(kj::ArrayPtr<const char> data);
Value parseJson(v8::Local<v8::String> text);
template <typename T>
Expand Down
Loading