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

Rework memory manager to make stack slot update faster #2813

Merged
merged 23 commits into from Apr 9, 2019

Conversation

@olonho
Copy link
Contributor

commented Mar 25, 2019

No description provided.

@olonho olonho force-pushed the cheaper_stack branch from b8e6a10 to 3fd7d8c Mar 27, 2019

@@ -455,6 +466,8 @@ extern "C" {
ObjHeader* result = name(__VA_ARGS__, OBJ_RESULT); \
return result; \
}
#define HEAP_RETURN_SLOT(slot) \

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Mar 27, 2019

Contributor

Why not enforce return slot to be always stack-located instead?
I believe that this is

  1. possible
  2. simpler
  3. more efficient

This comment has been minimized.

Copy link
@olonho

olonho Mar 27, 2019

Author Contributor

It's not just stack allocated, it must be in slots seen when walking the stack. Let's first stabilize this design and see its performance implications and the proceed with optimisations.

This comment has been minimized.

Copy link
@olonho

olonho Apr 9, 2019

Author Contributor

Did that.

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 9, 2019

Contributor

Did that help?

@olonho olonho force-pushed the cheaper_stack branch from 3fd7d8c to b076507 Mar 27, 2019

@olonho olonho changed the title Cheaper stack Rework memory manager to make stack slot update faster Mar 27, 2019

@olonho olonho force-pushed the cheaper_stack branch 3 times, most recently from d28e986 to 309c62e Mar 31, 2019

// We do not use cycle collector for frozen objects, as we already detected
// possible cycles during freezing.
// Also do not use cycle collector for provable acyclic objects.
int color = container->color();
if (color != CONTAINER_TAG_GC_PURPLE && color != CONTAINER_TAG_GC_GREEN) {
RuntimeAssert(!container->shareable(), "Shareable cannot be cycle collected");

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 1, 2019

Contributor

Isn't it the same as checked above?

This comment has been minimized.

Copy link
@olonho

olonho Apr 3, 2019

Author Contributor

Reworked.

inline void initThreshold(MemoryState* state, uint32_t gcThreshold) {
state->gcThreshold = gcThreshold;
state->toFree->reserve(gcThreshold);
state->toRelease->reserve(gcThreshold);

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 1, 2019

Contributor

Why was toFree->reserve(gcThreshold) removed?

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 1, 2019

Contributor

Do you suppose that it is unlikely to get filled up to threshold because toRelease will become full earlier?

This comment has been minimized.

Copy link
@olonho

olonho Apr 1, 2019

Author Contributor

Yes, it's no longer even close to that threshold.

header_ = AllocContainer(alloc_size);
RuntimeCheck(header_ != nullptr, "Cannot alloc memory");

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 1, 2019

Contributor

So the check below isn't required anymore?


#if GC_ERGONOMICS
auto gcStartTime = konan::getTimeMicros();
#endif

state->gcInProgress = true;

incrementStack(state);

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 1, 2019

Contributor

Do I understand correctly that long-living stack references become cycle candidates during every GC?

This comment has been minimized.

Copy link
@olonho

olonho Apr 3, 2019

Author Contributor

Exactly. Any alternatives?

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 4, 2019

Contributor

Not yet, but I believe we should eventually apply some efforts to find some.

@@ -1638,8 +1800,7 @@ void Kotlin_native_internal_GC_resume(KRef) {
MemoryState* state = memoryState;
if (state->gcSuspendCount > 0) {
state->gcSuspendCount--;
if (state->toFree != nullptr &&
freeableSize(state) >= state->gcThreshold) {
if (state->toFree != nullptr && freeableSize(state) >= state->gcThreshold) {

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 1, 2019

Contributor

Btw, I guess gcSuspendCount == 0 should be checked here too.

@@ -1650,6 +1811,7 @@ void Kotlin_native_internal_GC_stop(KRef) {
#if USE_GC
if (memoryState->toFree != nullptr) {
GarbageCollect();
// TODO: what shall we do with toRelease here? Just leak?

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 1, 2019

Contributor

Btw, this GC.start/stop feature doesn't seem to work properly.
Consider e.g.

kotlin.native.internal.GC.stop()
Any().freeze()

This comment has been minimized.

Copy link
@olonho

olonho Apr 3, 2019

Author Contributor

Yeah, more testing is needed, not sure if in this PR.

}
// Notify the future.
job.future->storeResultUnlocked(result, ok);
resultHolder.clear();

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 1, 2019

Contributor

As far as I see, by this moment another thread may have got this object already, so clearing the holder here may cause data race.

Btw, can transfer have ObjHolder parameter instead and clear it by itself? This looks making sense to me.

@olonho olonho force-pushed the cheaper_stack branch from ee29898 to 74c1a6d Apr 3, 2019

if (stackReferences * 5 > state->gcThreshold) {
auto newThreshold = state->gcThreshold * 3 / 2 + 1;
if (newThreshold < kMaxErgonomicThreshold) {
state->gcThreshold = newThreshold;

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 4, 2019

Contributor

Is space in toRelease not reserved intentionally?

This comment has been minimized.

Copy link
@olonho

olonho Apr 5, 2019

Author Contributor

Reworked here.

memoryState->toFree = nullptr;
memoryState->roots = nullptr;
state->gcSuspendCount = 0;

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 4, 2019

Contributor

What if GC is stopped while suspended?

This comment has been minimized.

Copy link
@olonho

olonho Apr 5, 2019

Author Contributor

Hard to tell what is right, but moved zeroing above.

state->toFree->push_back(container);
if (state->gcSuspendCount == 0 && state->toFree->size() >= state->gcThreshold) {
GC_LOG("Calling GC from DecrementRC: %d\n", state->toRelease->size())
garbageCollect(state, false);

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 5, 2019

Contributor

This DecrementRC variant is used only during GC. What's the purpose of invoking GC inside it?

@@ -111,9 +111,10 @@ volatile int allocCount = 0;
volatile int aliveMemoryStatesCount = 0;

// Forward declarations.
__attribute__((noinline))

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 8, 2019

Contributor

NO_INLINE?

if (container->hasContainerSize() && container->containerSize() == size) {
// TODO: shall it be == instead?
if (container->hasContainerSize() &&
container->containerSize() >= size && container->containerSize() <= 2 * size) {

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 8, 2019

Contributor

container->containerSize() <= 2 * size is too permissive. Can it be more like container->containerSize() <= size + 8?

if (old != nullptr ) {
ReleaseStackRef(old);
}
if (object != nullptr) {

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 8, 2019

Contributor

Does removing old != object check have significant positive effect?

This comment has been minimized.

Copy link
@olonho

olonho Apr 9, 2019

Author Contributor

Just remove excessive branch, but can return.

@olonho olonho force-pushed the cheaper_stack branch 2 times, most recently from 21970da to d7dd6d2 Apr 8, 2019

// Class holding reference to an object, holding object during C++ scope.
class ObjHolder {
public:
ObjHolder() : obj_(nullptr) {}
ObjHolder() : obj_(nullptr) {

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 9, 2019

Contributor

So it is actually now more like ObjStackHolder, StackRefHolder or even StackFrame.

@olonho olonho force-pushed the cheaper_stack branch from 87521e9 to 87cc869 Apr 9, 2019

@@ -62,10 +62,11 @@ enum {
THREAD_LOCAL_VARIABLE KInt g_currentWorkerId = 0;

KNativePtr transfer(ObjHolder* holder, KInt mode) {
holder->hold();

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 9, 2019

Contributor

Why not just

void* result = CreateStablePointer(holder.obj())
if (!ClearSubgraphReferences(holder->obj(), mode == CHECKED)) {
  ThrowWorkerInvalidState();
}
holder.clear()
return result;

Technically the same, but avoids leaking abstractions and pairs with AdoptStablePointer semantically.

@olonho olonho force-pushed the cheaper_stack branch from 36bd844 to 932d1f4 Apr 9, 2019

@olonho olonho force-pushed the cheaper_stack branch from 932d1f4 to 30be336 Apr 9, 2019

@olonho olonho merged commit d072920 into master Apr 9, 2019

@olonho olonho deleted the cheaper_stack branch Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.