[TIMOB-26242] Android: Attempt to fix memory behaviour #10221
Conversation
Generated by 🚫 dangerJS |
garymathews
force-pushed
the
TIMOB-26242
branch
2 times, most recently
from
a1de62e
to
0fa2ad3
Compare
garymathews
force-pushed
the
TIMOB-26242
branch
4 times, most recently
from
7d103e2
to
7e5ba71
Compare
|
FR Passed. Ran previous test cases & test case in the ticket & GC seems to work as expected. Studio Ver: 5.1.0.201807191252 |
There was a problem hiding this comment.
So I see what you're aiming for here, but I still think it's not necessarily the right way to do things.
It looks like you're basically offering up a sacrificial lamb to the JVM to GC and when it does you then tell V8 that we're low on memory and it tries to aggressively GC itself (at least on the next idle callback in the JVM). So in a way, you're sort of syncing up the GC on each side, but this seems pretty brute force.
I do think that the memory model/management is difficult to deal with now and I was just trying to get something working when I updated V8 long ago.
I think the fundamental issue here is that we have two GC'ing VMs that each have references to one half of the full object which lives across environments.
To my mind, that means we should be keeping objects alive on both halves until each side has agreed that they don't need it anymore. Or else we get JS objects with no backing Java proxy, or Java proxies whose JS object/pointer has been cleared.
We've been trying to get this right with reference tables using weak/soft/strong references, etc. I'm still not sure we're doing it right.
From the looks of the recents string of PRs, it appears we're not.
Your flowchart here does represent the ideal scenario but only from the V8 side. We create a JS object and a Java proxy, stick a (strong) reference to the Java proxy in a Map, mark the JS object as weak/independent and wait for a V8 GC callback.
When that comes, we revive the JS object and move the Java reference to become weak and then wait for the JVM to GC it. We then wait for finalize() to get called on the object, call V8Object.nativeRelease() and that will delete the native proxy if the JS side is "detached".
So I think there's a handful of things we haven't really looked into here:
- What if we get to the end and the native proxy/JS object is not "detached"? Won't it stay alive in V8 forever? (and be dead on the JVM side?)
- What if we want to initiate the cleanup from the JVM side? It looks like the reflection stuff in KrollObject was intended to try and fix that?
I think we basically need to have a two-pass GC system on both native and JVM sides; that we need to simplify the code on the native side; and that neither side gets to get fully GC'd unless both sides agree for the same object.
We have no real mechanism for "reviving" the Java half if the JVM wants to GC it. It (may) call finalize() and try to release the JS half, but if the JS half hasn't been asked to be GC'd yet, it won't be.
| double deadline_in_s = V8Runtime::platform->MonotonicallyIncreasingTime() + 0.1; | ||
| return V8Runtime::v8_isolate->IdleNotificationDeadline(deadline_in_s); | ||
| // notify V8 of low memory to suggest a full GC | ||
| V8Runtime::v8_isolate->LowMemoryNotification(); |
There was a problem hiding this comment.
I think this is a pretty fundamentally different method to be calling here.
This basically informs that memory is low and V8 should try to GC pretty aggressively, which isn't necessarily the case here.
I think really this method should take the deadline increment as an arg and the Java side should probably use some sort of backoff function to increasingly allocate more time to do GC so long as we're still idle and the idle notification deadline call returns false (meaning it wants to do more GC).
| krollObject.release(); | ||
| } | ||
| if (krollObject != null) { | ||
| krollObject.release(); |
There was a problem hiding this comment.
I certainly think that this revert cleans up a not so elegant change that was made to try and fix memory... so +1 to making it cleaner.
|
@sgtcoolguy I'll try to explain what's happening here and the reasons behind the changes in this PR which should answer some of your concerns. The idea here is to have JVM as the dominant GC because it takes into account the actual memory consumption of the allocated objects. However, due to the ReferenceTable JVM cannot collect objects currently referenced by V8. This is why we need to suggest V8 to do a full GC with This is why it's hard to make each runtime agree that an object can be collected. V8 may never agree some objects should be collected as V8 can't see the actual resources they are consuming. It only sees small proxy object that may not be worth its time to GC. When in reality, that object could be consuming lots of memory in JVM. That's why I'm forcing V8 to do a full collection. You could say doing this is allowing both runtimes to agree an object can be collected, with JVM getting its confirmation from V8s GC. Forcing V8 to do a full GC isn't too taxing either as the objects its collecting are small in size. And it will only do a GC at the intervals of JVMs GC (which would be at idle times).
Previously, I wrote some poor workarounds to get references in the ReferenceTable to downgrade and be collected. This was before I understood what was happening (or not happening) with V8 GC's; that V8 was too lax with its collections. |
|
So, certainly the JVM is aware of how big all the Java objects are - but it's unaware of how much memory V8 is taking, and vice versa. There is an API to try and inform V8 about external memory being held by objects: We likely should be trying to make use of that. I would have assumed perhaps there'd be a way to associate specific objects with external memory, but I couldn't find that. But also, it may be rather difficult to get the size of java objects quickly/easily. I do see a jvmti method for it, but that is meant for tooling (i.e. debuggers/profilers). I suppose given our current state, this PR seems Ok, but again I am very hesitant to be moving from our idle notifications to a full out low memory notification. If called enough (or with enough time for it's deadline) the idle notification can also trigger full GCs. I tried to clean this code up locally, but it's a jumble. One thing I think we can do is remove the useGlobalRefs code branches since it seems we've already modified the code so it's always false now... Also The KrollProxy/ReferenceTable passing of reference keys can now be removed as it's unused (since you removed the reflection hack here). |
|
I did look at AdjustAmountOfExternalAllocatedMemory(), but it would pretty much achieve the same result as what this PR does.
But we would have the added complexity of sampling our memory usage to update it. And a full GC may not happen soon enough, which could prevent functionality that requires large memory resources (image loading etc..) if memory is constraint. That's also why I opted not to use Another thought could be to change the GC constraints of V8, such as lowering the create_params.constraints.set_max_old_space_size(4); // 4MB |
|
@garymathews OK, I'm convinced 😄 Please remove the useGlobalRefs related code in JavaObject/V8Runtime since it's no longer used - should make the code a little easier to follow. Also, please remove the ReferenceTable/KrollProxy setReferenceKey method/calls since those are also now unused. Then I think we're good to merge. |
garymathews
force-pushed
the
TIMOB-26242
branch
from
8ec74e7
to
6f088a7
Compare
releaseKroll()are not necessary since the recent memory management refactorOBJECT LIFECYCLE
JIRA Ticket