Skip to content

Conversation

@stesie
Copy link
Member

@stesie stesie commented Mar 8, 2016

So far the size of an object exported to V8 was assumed to be 1k flat. If a lot of larger PHP objects were sequentially exported, then the "amount of external memory" considered by V8 is far too low and hence garbage collection due to "external memory allocation limit reached" is neither done to infrequently or never at all

The setAverageObjectSize method now allows to adjust this assumption

stesie added a commit that referenced this pull request Mar 8, 2016
add V8Js::setAverageObjectSize method
@stesie stesie merged commit 7ab602f into phpv8:master Mar 8, 2016
@pinepain
Copy link
Member

pinepain commented Mar 8, 2016

In case you set large avg obj size, then create object, and then set some small avg obj size, adjusting external memory will decreased by newer avg size. In case you create a lot of object with small avg size and then set large avg size, freeing that obj and adjusting memory back with larger avg size may lead to undefined behavior or errors. In my php-v8 ext I store avg obj size in obj struct (I use custom objects, so it's just a struct field) and then adjust external memory against that value.

@stesie
Copy link
Member Author

stesie commented Mar 9, 2016

yeah, I thought about that too, but came to the conclusion, that no one would want to adjust the avg size here and there. But you prove me wrong :-)
Since V8Js has a weakly persistent handle, it could actually store the estimated size along that one. And when finally freeing the handle, then adjust V8's amount of external memory by that.

Another point: what do you think of another means to directly influence the amount of external memory? Where you don't set a relative amount of memory, but an absolute one.
That way you could (every now and then) do

$v8->setExternalMemory(get_memory_usage(false));

... or so. Or you could deduct the amount of memory in use when the V8Js instance was constructed

@stesie stesie deleted the setaverageobjectsize branch January 20, 2017 17:16
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.

2 participants