[n00bish]

Closes #18
I've been working on adding this feature per conversations with @seanmakesgames.

I've added a callback to the GC Epilogue that checks percentage of heap used against a user-defined value (provided at Context creation). This will help prevent OOMs in well-behaved code, but can not detect or prevent large allocations causing an OOM to happen.

I've added two new isolate data buckets to store the softlimit threshold as well as a flag to show the softlimit was reached. I'm not convinced this is the best way as further features and flags could continue to pile on and create excessive GetData / SetData code blocks. My current thought for a better solution would be to pass around a struct via isolate Get/SetData that contains configuration and flag information.

My other point of uncertainty is whether or not stopping attached code is needed in the gc callback. Unless I am mistaken it should not be necessary as attached code will not be allocating in the V8 heap, so any V8 garbage collection events should be from eval'ed code only.

I also fixed a typo, and had to import two modules in the test class to get it to run. My apologies for bundling them in this PR, I would be happy to remove those changes if desired.

[seanmakesgames]

@SamSaffron This PR is a stopgap solution which works in the interim, until we figure out how the OOM handler added in 5.4 works. (and if it works)
Any concerns / comments you have are much appreciated.

[seanmakesgames]

@SamSaffron can we get a review on this? Would love to get your input, as there are some real finicky things in here. (and the integration of this code could instantly improve server performance by orders of magnitude)

[SamSaffron]

Sorry, its going to take me a while to get time to do a careful review here, I am knees deep in replacing our asset compiler for discourse :)

…

On Tue, Mar 14, 2017 at 4:46 PM, seanmakesgames ***@***.***> wrote: @SamSaffron <https://github.com/SamSaffron> can we get a review on this? Would love to get your input, as there are some real finicky things in here. (and the integration of this code could instantly improve server performance by orders of magnitude) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAUXUMFRC0VFpxTJ9yd3ff3618CeHkLks5rlvycgaJpZM4MYsUf> .

[seanmakesgames]

Yikes! (totally get that though) Thanks for the response. Really appreciate it. :)

[SamSaffron]

I absolutely want to have soft limits, but the implementation does make me a bit uneasy.

In particular ...

• I would like the API to be: Context.new(max_memory: 100.megabytes)

• Why not use AddMemoryAllocationCallback? then when you allocate we can do the check, how frequently is this called?

• If we are stuck using GC callbacks why the need for ratios here, why not just use one number?

[seanmakesgames]

iirc AddMemoryAllocationCallback does not work anymore- and this pr was just a stop-gap until (things that were added in 5.4) were available to us

[seanmakesgames]

Api change should totally work though :)

[SamSaffron]

I think we are fine to wait on 5.4 then, what is the solution in 5.4+?

…

On Mon, 17 Jul 2017 at 12:57 pm, seanmakesgames ***@***.***> wrote: Api change should totally work though :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAUXYvBHtVU1Oc4cWgvJU_sQLEwkiR4ks5sO5ERgaJpZM4MYsUf> .

[seanmakesgames]

It's been a while, so the convos are a bit buried, but there's an oomhandler of some kind in 5.4 that we could use. I have to dig it up again-- @n00bish found it last and suggested it. (we couldn't try it though, b/c of version availability)

[n00bish]

My apologies for not being more responsive - things have been rather busy for me. Going to try to chime in with what I remember, but I wrote this initially a little while ago so I'd need to take some time to review.

There may also be an issue with the OOM handler (isolate->SetOOMErrorHandler) depending on whether or not the OOM error is "thrown" (queued to throw) after the OOM handler is called, even if we clean up. If the OOMHandler is called pre-throw, we can potentially throw a different error that allows killing the currently running script and cleaning up in a way where we can reuse the isolate. Depending on this behavior, the error handler may not serve our purpose - some testing is needed.

The reason for ratios was simply the thought that as heap sizes can be variable a ratio could be easier to uniformly apply across multiple different sizes of heaps. I'm not too sold on that particular implementation so it can certainly be changed if ratios aren't the preferred configuration.

[SamSaffron]

Yeah I definitely want to try to see if we can swing this with SetOOMErrorHandler, since this is per isolate it is possible we can swing this as the mechanism for setting limits and just go with hardlimits instead.

Can we spin up multiple isolates with different hardlimits? Need to look at the internal apis ...

[seanmakesgames]

NoMethodError: undefined method 'megabytes' for 5:Fixnum
@n00bish was getting this
@SamSaffron is the .megabytes method only available after a certain ruby version / with a lib or somithing?

[SamSaffron]

megabytes is an active support thing, happy to remove the dependency from the test suite if you want to send a pr through

…

On Thu, Jul 27, 2017 at 1:15 PM, seanmakesgames ***@***.***> wrote: NoMethodError: undefined method 'megabytes' for 5:Fixnum @n00bish <https://github.com/n00bish> was getting this @SamSaffron <https://github.com/samsaffron> is the .megabytes method only available after a certain ruby version / with a lib or somithing? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAUXXrBLn4bI2taRbBGNguya9jq5PvNks5sSMW_gaJpZM4MYsUf> .

[seanmakesgames]

not super concerned, just wanted to get a better picture of what you wanted from the max_memory api, and what it might mean for dependencies.
@n00bish is cookin' up this PR, I'm just asking questions for him. ;)

[n00bish]

I took a look into the OOM api, and I don't think that will work for us. Here's the relevant code that is called to execute the handler provided:
https://github.com/v8/v8/blob/master/src/api.cc#L405

As you can see, the code executes your handler then signals a fatal error, which is what we're trying to avoid. I'm not sure if throwing another error within the handler would prevent the fatal error from being signaled and allow us to clean up, but that feels like a nasty hack if it even worked. Thoughts?

[SamSaffron]

Ouch, that is no fun. I guess we are stuck with - If soft limit is set - After GC runs - Check memory being used - If it is >= to soft limit - dispose context - raise error API wise I would go with # where limit is the number of bytes you want to limit it at MiniRacer::Context.new(max_heap_size: 100000) No need of any other thresholds etc here, I vote for keeping it simple.

…

On Thu, Jul 27, 2017 at 4:52 PM, n00bish ***@***.***> wrote: I took a look into the OOM api, and I don't think that will work for us. Here's the relevant code that is called to execute the handler provided: https://github.com/v8/v8/blob/master/src/api.cc#L405 As you can see, the code executes your handler then signals a fatal error, which is what we're trying to avoid. I'm not sure if throwing another error within the handler would prevent the fatal error from being signaled and allow us to clean up, but that feels like a nasty hack if it even worked. Thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAUXZQA9ahzKB1l1_SfMa7n4hoRdUReks5sSPiQgaJpZM4MYsUf> .

[seanmakesgames]

I'm on board with this. (means you've also almost done this already @n00bish; just need to move some parts around in this pr) :)

[n00bish]

@SamSaffron I've got the implementation updated but I've been hashing out a segfault with @seanmakesgames and I can't quite determine the root cause. Here's the branch with the offending code: https://github.com/n00bish/mini_racer/tree/segfaults

When you set the value for the memory softlimit test to 2550000, it will succeed - but at 2560000 or above, it segfaults. The stack traces via gdb show the following:

Core was generated by `/usr/bin/ruby2.3 -w -Ilib:test:lib -I/var/lib/gems/2.3.0/gems/rake-12.0.0/lib /'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007ff12bfd57c3 in v8::internal::Script::GetWrapper(v8::internal::Handle<v8::internal::Script>) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
[Current thread is 1 (Thread 0x7ff12fb38700 (LWP 5612))]
(gdb) bt
#0  0x00007ff12bfd57c3 in v8::internal::Script::GetWrapper(v8::internal::Handle<v8::internal::Script>) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
#1  0x00007ff12bfc252e in v8::internal::MessageHandler::MakeMessageObject(v8::internal::Isolate*, v8::internal::MessageTemplate::Template, v8::internal::MessageLocation const*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::JSArray>) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
#2  0x00007ff12bf90a76 in v8::internal::Isolate::CreateMessage(v8::internal::Handle<v8::internal::Object>, v8::internal::MessageLocation*) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
#3  0x00007ff12bf9211e in v8::internal::Isolate::Throw(v8::internal::Object*, v8::internal::MessageLocation*) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
#4  0x00007ff12bf9258b in v8::internal::Isolate::ScheduleThrow(v8::internal::Object*) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
#5  0x00007ff12bd2ee56 in v8::Isolate::ThrowException(v8::Local<v8::Value>) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
#6  0x00007ff12bd04591 in gc_callback (isolate=0x164e0e0, type=<optimized out>, flags=<optimized out>) at ../../../../ext/mini_racer_extension/mini_racer_extension.cc:137
#7  0x00007ff12bf15ffe in v8::internal::Heap::CallGCEpilogueCallbacks(v8::GCType, v8::GCCallbackFlags) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
#8  0x00007ff12bf2c08c in v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
#9  0x00007ff12bf2c766 in v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector, v8::internal::GarbageCollectionReason, char const*, v8::GCCallbackFlags) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
#10 0x00007ff12bedb850 in v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so
#11 0x00007ff12c3ba1d2 in v8::internal::Runtime_AllocateInNewSpace(int, v8::internal::Object**, v8::internal::Isolate*) () from /home/n00bish/git/mini_racer/lib/mini_racer_extension.so

The output from the test case including the debug output I added shows:

gc callback called, softlimit is 5000000 and used is 2077208
gc callback called, softlimit is 5000000 and used is 2157240
gc callback called, softlimit is 5000000 and used is 2557320
gc callback called, softlimit is 5000000 and used is 2957320
gc callback called, softlimit is 5000000 and used is 25540376
Received signal 11 SEGV_MAPERR 000000000027

Any idea what the problem could be?

[SamSaffron]

hmmm, maybe you are not allowed to throw there? maybe try to dispose the whole context at that point and flag the reason as out of memory?

[SamSaffron]

also, please post this on the v8 google group to see if anyone there can help ?

[n00bish]

I just made some changes to the code in the PR which should address the most recent issues and make this ready for another round of review.

I went for a slightly different approach by removing the throw from the gc callback, allowing the running script to be terminated by V8's normal cleanup process. This removes the "hard stop" that the throw was providing, but when working with normal (not extremely small) heap sizes it should terminate in time to prevent an OOM when reasonable softlimits are set.

In most scenarios execution will terminate before an OOM occurs, but an OOM could still occur in a scenario where the code was designed to allocate extremely large chunks of memory repeatedly, but that seems unlikely to be the case for "normal usage". Tracking memory usage on that level would require access to the memory allocation callback API, which was removed in an earlier version of V8.

[SamSaffron]

I would soft limit at 200mb cause 1gb is a bit high for the test it is probably going to fail on travis at some point

[SamSaffron]

This PR is looking great to me, just make that minor change squash your commits, add documentation to readme.md and we are off to the races!

[n00bish]

@SamSaffron should be ready to go!

[SamSaffron]

Awesome! thank you so much, this is a hugely welcome change.

[tisba]

This is great, thanks @n00bish! ❤️