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

Work around out of memory issue with foreign allocator. #199

Merged
merged 5 commits into from
Oct 28, 2019

Conversation

jmbr
Copy link
Contributor

@jmbr jmbr commented Oct 24, 2019

Work around for #198.

(yason:encode t s))))))))

#+sbcl
(sb-ext:gc :full t))
Copy link
Contributor

Choose a reason for hiding this comment

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

qvm-app already depends on trivial-garbage, so maybe trivial-garbage:gc instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay.

Copy link
Contributor

@appleby appleby left a comment

Choose a reason for hiding this comment

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

unsatisfying in the long term, but seems reasonable as a 🔥hot fix 🔥.

(yason:encode t s)))))))))
(yason:encode t s))))))))

(tg:gc :full t))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe leave a comment indicating why this is required

@stylewarning
Copy link
Member

Maybe we should explicitly deallocate instead of relying on the GC? It's scary that every request calls a GC which locks all threads.

@stylewarning stylewarning self-requested a review October 24, 2019 23:13
@jmbr
Copy link
Contributor Author

jmbr commented Oct 24, 2019

Maybe we should explicitly deallocate instead of relying on the GC? It's scary that every request calls a GC which locks all threads.

Right now this is only triggered when using foreign memory allocation and most users will be resorting to lisp allocation for the foreseeable future. I agree with you but I think the current patch is OK as a transient fix and I will let someone else (perhaps my future, not-on-vacation self?) produce a more definitive fix.

@stylewarning
Copy link
Member

I'll accept it if you write somewhere that it's a transient fix in the source.

@stylewarning
Copy link
Member

@jmbr

@jmbr
Copy link
Contributor Author

jmbr commented Oct 25, 2019

@jmbr

ROFL. Deal!

P.S.: See also #200

@jmbr
Copy link
Contributor Author

jmbr commented Oct 25, 2019

Let me give this another go later before merging.

@appleby
Copy link
Contributor

appleby commented Oct 27, 2019

Happened upon this paper today which touches on some related GC themes:

Dynamic Optimizations for SBCL Garbage Collection

The paper is short and light on details, but all three of the optimizations mentioned seem like things qvm-app might potentially benefit from (with the huge caveat that the methods hinted at in the paper rely on running inside a container--though it isn't immediately clear that they wouldn't/couldn't work in a non-containerized environment).

Relevant snippet from the paper:

3.1 Optimization # 1: Trigger GC based on host memory constraints

The first optimization is to use memory utilization metrics to indicate to the lisp runtime when collecting garbage is actually required to avoid an out of memory condition. This entails configuring up to two memory thresholds. For an application running within a container, these thresholds are most logically expressed as a fraction of the container’s total memory size. When the container’s memory in use crosses a specified threshold, the lisp process is notified of the impending need to collect garbage. These triggers may replace the standard behavior of beginning garbage collection after allocating a fixed number of bytes on the lisp heap.

In the paper, this is paired with a couple of other optimizations, one of which is to delay promoting objects out of the nursery if GC happens while an application request is in process. In the case of qvm-app, this would presumably mean that the QVM object remains in gen0 until after the request is finished, at which point it would no longer be live and get collected.

When the GC runs while one or more work items are in progress, only generation 0 is scavenged, and no surviving objects are promoted to an older generation. Heap objects which survive a gen 0 GC are presumed to be logically associated with in-progress work items and are expected to be garbage when those work items complete.

For the record, I'm not proposing we implement any of these for qvm-app. I just happened on the paper and the theme was topical/seemed worth sharing. Maybe stylewarning even remembers the talk and/or got a t-shirt 👕.

@jmbr
Copy link
Contributor Author

jmbr commented Oct 28, 2019

Happened upon this paper today which touches on some related GC themes:

Dynamic Optimizations for SBCL Garbage Collection

Thanks for referencing that paper. I have read it and agree that it could be useful.

The problem here is indeed that the GC is unaware of the true extent of a QVM object (in particular, its foreign-allocated vector of amplitudes) and that the QVM object survives for many generations. Triggering the GC based on the current process' resident segment size as suggested in the paper is a good solution. It is, however, unportable across Lisp implementations and operating systems. For instance, SBCL can use GENCGC on some architectures and CheneyGC on others whereas ECL uses the Boehm garbage collector. Also, the typical way to query available memory in Linux is via the proc filesystem whereas the mechanism differs in macOS (and Windows). One could create a thread that periodically monitors memory usage and manually triggers a full GC when necessary but I think a simpler solution to fix this properly is to manually free the amplitudes vector as soon as possible after finishing the request.

@appleby
Copy link
Contributor

appleby commented Oct 28, 2019

I think a simpler solution to fix this properly is to manually free the amplitudes vector as soon as possible after finishing the request.

Yep, I agree.

@notmgsk
Copy link
Member

notmgsk commented Oct 28, 2019

I think a simpler solution to fix this properly is to manually free the amplitudes vector as soon as possible after finishing the request.

Yep, I agree.

I concur.

@appleby
Copy link
Contributor

appleby commented Oct 28, 2019

I think a simpler solution to fix this properly is to manually free the amplitudes vector as soon as possible after finishing the request.

Yep, I agree.

I concur.

🤝

@stylewarning stylewarning merged commit 5fd4492 into master Oct 28, 2019
appleby added a commit that referenced this pull request Oct 30, 2019
The garbage collection added at the end of HANDLE-REQUEST in #199
prevents HANDLE-REQUEST from returning a value. Some of the handlers
write to the response stream directly, but others just return a value,
which hunchentoot handles sending back to the client.
appleby added a commit that referenced this pull request Oct 30, 2019
The garbage collection added at the end of HANDLE-POST-REQUEST in #199
prevents HANDLE-POST-REQUEST from returning a value. Some of the
handlers write to the response stream directly, but others just return
a value and let HUNCHENTOOT handle sending the response back to the
client.

This commit (1) ensures that a response value is always returned from
HANDLE-POST-REQUEST, and (2) ensures that cleanup / gc is always run,
even if the request signals an error.
stylewarning pushed a commit that referenced this pull request Oct 31, 2019
The garbage collection added at the end of HANDLE-POST-REQUEST in #199
prevents HANDLE-POST-REQUEST from returning a value. Some of the
handlers write to the response stream directly, but others just return
a value and let HUNCHENTOOT handle sending the response back to the
client.

This commit (1) ensures that a response value is always returned from
HANDLE-POST-REQUEST, and (2) ensures that cleanup / gc is always run,
even if the request signals an error.
@notmgsk notmgsk removed the / ! \ DO NOT MERGE / ! \ Do not merge! label Nov 5, 2019
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.

4 participants